[{"id":17736,"web_url":"https://patchwork.libcamera.org/comment/17736/","msgid":"","date":"2021-06-24T11:45:37","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"HI JM,\n\nfew minors below:\n\nOn 6/22/21 3:15 PM, Jean-Michel Hautbois wrote:\n> For various sensor operations, it may be needed to do sensor specific\n> computations, like analogue gain or vertical blanking.\n>\n> This commit introduces a new camera sensor helper in libipa which aims\n> to solve this specific issue.\n> It is based on the MIPI alliance Specification for Camera Command Set\n> and implements, for now, only the analogue \"Global gain\" mode.\n> Setting analogue gain for a specific sensor is not a straightforward\n> operation, as one needs to know how the gain is calculated for it.\n>\n> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>\n> Adding a new sensor is pretty straightforward as one only needs to\n> implement the sub-class for it and register that class to the\n> CameraSensorHelperFactory.\n>\n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 87 +++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 407 insertions(+)\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..a114ee73\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,318 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Helper class that performs sensor-specific parameter computations\n> + *\n> + * Computation of sensor configuration parameters is a sensor specific\n> + * operation. Each CameraHelper derived class computes the value of\n> + * configuration parameters, for example the analogue gain value, using\n> + * sensor-specific functions and constants.\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n\n+1\n\nthis is nice and how each CameraSensorHelper is registered down below. \nThe usage of this macro is apt.\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class CameraSensorHelper\n> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n> + *\n> + * CameraSensorHelper derived class instances are sensor specific. Each\n\nminor: I would write this as for clarity:\n\n * Instances derived from CameraSensorHelper class are sensor specific. Each\n\n> + * supported sensor will have an associated base class defined.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + *\n> + * CameraSensorHelper derived class instances shall never be constructed manually\n> + * but always through the CameraSensorHelperFactory::create() method.\n> + */\n> +\n> +/**\n> + * \\brief Compute gain code from the analogue gain absolute value\n> + * \\param[in] gain The real gain to pass\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The gain code to pass to V4l2\n> + */\n> +uint32_t CameraSensorHelper::gainCode(double gain) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n\nThis won't yield a floating value, ever? In case, it does, are we OK \nreturning it as uint32_t with loss of precision. I am not sure, I \nhaven't read the spec or understand this (maybe it's right, since it's \ngain-\"code\")\n\n\n> +}\n> +\n> +/**\n> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of the function\n> + * CameraSensorHelper::gainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The real gain\n> + */\n> +double CameraSensorHelper::gain(uint32_t gainCode) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> +\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> +}\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainType\n> + * \\brief the gain calculation modes as defined by the MIPI CCS\ns/the/The\n> + *\n> + * Describes the image sensor analogue gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainLinear\n> + * \\brief Gain is computed using linear gain estimation\n> + *\n> + * The relationship between the integer gain parameter and the resulting gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> + *\n> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> + * image-sensor-specific constants of the sensor.\n> + * These constants are static parameters, and for any given image sensor either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainExponential\n> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n> + * If the image sensor supports it, then the global analogue gain can be controlled\n> + * by linear and exponential gain formula:\n> + *\n> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n> + * \\todo not implemented in libipa\n> + */\n> +\n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainConstants\n> + * \\brief Analogue gain constants used for gain calculation\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n> + * \\brief Analogue gain calculation mode\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> + * \\brief Constant used in the analogue Gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\ns/either/Either\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\nSame\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using the\ns/classes./sub-classes. maybe?\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + * \\param[in] name Name of the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +{\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactory::factories();\n> +\n> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> +\t\tif (name != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> +\t\treturn std::unique_ptr(helper);\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor helper class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +{\n> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor helper factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return The list of camera sensor helper factories\n> + */\n> +std::vector &CameraSensorHelperFactory::factories()\n> +{\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelperFactory::name_\n> + * \\brief The name of the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> + * try and match devices.\n\nI do wonder about the reasons that registering a camera sensor helper \nneeds to create a derived factory class first\nand  then, it is responsible to create Instance(s) of that camera sensor \nhelper per se?\n\nCan CameraSensorHelper base class be flexible enough to create those \ninstances by itself and keeping the registry internal?\nI am not sure, there might be some road-blocks I am not able to see \nright now.\n\n.... but this isn't that complicated either :-)\n\nSo,\n\nReviewed-by: Umang Jain \n\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperImx219\n> + * \\brief Create and give helpers for the imx219 sensor\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..c43e4bf6\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelper() = default;\n> +\tvirtual ~CameraSensorHelper() = default;\n> +\n> +\tvirtual uint32_t gainCode(double gain) const;\n> +\tvirtual double gain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainType {\n> +\t\tAnalogueGainLinear = 0,\n> +\t\tAnalogueGainExponential = 2,\n> +\t};\n> +\n> +\tstruct AnalogueGainConstants {\n> +\t\tAnalogueGainType type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\n> +\tAnalogueGainConstants analogueGainConstants_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(const std::string &name);\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +protected:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t \\\n> +class helper##Factory final : public CameraSensorHelperFactory\t \\\n> +{ \\\n> +public: \\\n> +\thelper##Factory() : CameraSensorHelperFactory(name) {}\t \\\n> +\t\t\t\t\t\t\t\t \\\n> +private: \\\n> +\tCameraSensorHelper *createInstance()\t\t\t \\\n> +\t{\t\t\t\t\t\t\t \\\n> +\t\treturn new helper();\t\t\t\t \\\n> +\t}\t\t\t\t\t\t\t \\\n> +};\t\t\t\t\t\t\t\t \\\n> +static helper##Factory global_##helper##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 038fc490..dc90542c 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,11 +2,13 @@\n> \n> libipa_headers = files([\n> 'algorithm.h',\n> + 'camera_sensor_helper.h',\n> 'histogram.h'\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'camera_sensor_helper.cpp',\n> 'histogram.cpp'\n> ])\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 903EDC321A\n\tfor ;\n\tThu, 24 Jun 2021 11:45:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D65026845A;\n\tThu, 24 Jun 2021 13:45:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 817B96050B\n\tfor ;\n\tThu, 24 Jun 2021 13:45:44 +0200 (CEST)","from [192.168.0.104] (unknown [103.251.226.162])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E63B0532;\n\tThu, 24 Jun 2021 13:45:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Vn3fVyd0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624535144;\n\tbh=Z01IXBVzmmorMyb4sBtAFI35k7Yeikb2hdKhd78nrcw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Vn3fVyd0449QiisWHJEa/vbV5TGOlOVyKub8luHuZACYrKPLK+xfvpXJr6d6GCBR8\n\tMOrMjEjw+ma+Prjzaui+7pO0kQF7VGAJl7jiiZHiB+LO/XMrn8MgdGUu7auyOERuqA\n\t/Gs0meaj73K7Tl6kPOzKUj7CY8wtFnvXAuOpEds0=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Thu, 24 Jun 2021 17:15:37 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17738,"web_url":"https://patchwork.libcamera.org/comment/17738/","msgid":"","date":"2021-06-24T12:46:08","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 22/06/2021 10:45, Jean-Michel Hautbois wrote:\n> For various sensor operations, it may be needed to do sensor specific\n> computations, like analogue gain or vertical blanking.\n> \n> This commit introduces a new camera sensor helper in libipa which aims\n> to solve this specific issue.\n> It is based on the MIPI alliance Specification for Camera Command Set\n> and implements, for now, only the analogue \"Global gain\" mode.\n> Setting analogue gain for a specific sensor is not a straightforward\n> operation, as one needs to know how the gain is calculated for it.\n> \n> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n> \n> Adding a new sensor is pretty straightforward as one only needs to\n> implement the sub-class for it and register that class to the\n> CameraSensorHelperFactory.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 87 +++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 407 insertions(+)\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..a114ee73\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,318 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Helper class that performs sensor-specific parameter computations\n> + *\n> + * Computation of sensor configuration parameters is a sensor specific\n> + * operation. Each CameraHelper derived class computes the value of\n> + * configuration parameters, for example the analogue gain value, using\n> + * sensor-specific functions and constants.\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class CameraSensorHelper\n> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n> + *\n> + * CameraSensorHelper derived class instances are sensor specific. Each\n> + * supported sensor will have an associated base class defined.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + *\n> + * CameraSensorHelper derived class instances shall never be constructed manually\n> + * but always through the CameraSensorHelperFactory::create() method.\n> + */\n> +\n> +/**\n> + * \\brief Compute gain code from the analogue gain absolute value\n> + * \\param[in] gain The real gain to pass\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The gain code to pass to V4l2\n> + */\n> +uint32_t CameraSensorHelper::gainCode(double gain) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n\nThis doesn't need brackets around the two comparisons.\n\nThe == takes precedence over the || so those are evaluated before the\nlogical OR.\n\nhttps://en.cppreference.com/w/cpp/language/operator_precedence\n\nI personally don't mind over-scoping though, as it helps make sure\nthings are clear when order of precedence isn't always in peoples minds.\n\n\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +}\n> +\n> +/**\n> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of the function\n> + * CameraSensorHelper::gainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The real gain\n> + */\n> +double CameraSensorHelper::gain(uint32_t gainCode) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n\nSame,\n\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> +\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n\nWe're at v6, so I assume those calculations have been verified by now?\n\nIn particular all the type promotions ...\n\n\n> +}\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainType\n> + * \\brief the gain calculation modes as defined by the MIPI CCS\n> + *\n> + * Describes the image sensor analogue gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainLinear\n> + * \\brief Gain is computed using linear gain estimation\n> + *\n> + * The relationship between the integer gain parameter and the resulting gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> + *\n> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> + * image-sensor-specific constants of the sensor.\n> + * These constants are static parameters, and for any given image sensor either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainExponential\n> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n> + * If the image sensor supports it, then the global analogue gain can be controlled\n> + * by linear and exponential gain formula:\n> + *\n> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n> + * \\todo not implemented in libipa\n> + */\n> +\n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainConstants\n> + * \\brief Analogue gain constants used for gain calculation\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n> + * \\brief Analogue gain calculation mode\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> + * \\brief Constant used in the analogue Gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + * \\param[in] name Name of the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +{\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactory::factories();\n> +\n> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> +\t\tif (name != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> +\t\treturn std::unique_ptr(helper);\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor helper class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +{\n> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor helper factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return The list of camera sensor helper factories\n> + */\n> +std::vector &CameraSensorHelperFactory::factories()\n> +{\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelperFactory::name_\n> + * \\brief The name of the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> + * try and match devices.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperImx219\n> + * \\brief Create and give helpers for the imx219 sensor\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n\nIf these will grow, should we have one file per camera sensor helper to\nsplit these out ?\n\nOr is that overkill for the moment? I guess it can be broken out later\nwhen new features are added that would make the scale increase.\n\n\nOther than that, I don't want to block this series for things that can\ncontinue in development so:\n\nReviewed-by: Kieran Bingham \n\nand I'll let you decide if it should be split now or later.\n\n--\nKieran\n\n\n\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..c43e4bf6\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelper() = default;\n> +\tvirtual ~CameraSensorHelper() = default;\n> +\n> +\tvirtual uint32_t gainCode(double gain) const;\n> +\tvirtual double gain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainType {\n> +\t\tAnalogueGainLinear = 0,\n> +\t\tAnalogueGainExponential = 2,\n> +\t};\n> +\n> +\tstruct AnalogueGainConstants {\n> +\t\tAnalogueGainType type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\n> +\tAnalogueGainConstants analogueGainConstants_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(const std::string &name);\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +protected:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t \\\n> +class helper##Factory final : public CameraSensorHelperFactory\t \\\n> +{ \\\n> +public: \\\n> +\thelper##Factory() : CameraSensorHelperFactory(name) {}\t \\\n> +\t\t\t\t\t\t\t\t \\\n> +private: \\\n> +\tCameraSensorHelper *createInstance()\t\t\t \\\n> +\t{\t\t\t\t\t\t\t \\\n> +\t\treturn new helper();\t\t\t\t \\\n> +\t}\t\t\t\t\t\t\t \\\n> +};\t\t\t\t\t\t\t\t \\\n> +static helper##Factory global_##helper##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 038fc490..dc90542c 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,11 +2,13 @@\n> \n> libipa_headers = files([\n> 'algorithm.h',\n> + 'camera_sensor_helper.h',\n> 'histogram.h'\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'camera_sensor_helper.cpp',\n> 'histogram.cpp'\n> ])\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6EA2FC321A\n\tfor ;\n\tThu, 24 Jun 2021 12:46:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CED6D68455;\n\tThu, 24 Jun 2021 14:46:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5EEBE6050B\n\tfor ;\n\tThu, 24 Jun 2021 14:46:12 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE8964A1;\n\tThu, 24 Jun 2021 14:46:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MzdrnieK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624538772;\n\tbh=Y13G22XF23avcKPDuUBwW9a+4eNkvE89XbSE3ABaLoU=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=MzdrnieKwZVwDSp8e4/Nk8QYhf/4LudOXJeDskHApi1SaCUj+eJBlrI3azOYXeEyW\n\t5FyIcICnlWg4u9msS4BpZ+l5Pg+PUfiQqJun4MFRjYrSuiBJPOQcCjRSkeqS8MY6c1\n\tt1STMEApXJna2NxSgyIwsl7hbUb5MwjU7Cbp8msc=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Thu, 24 Jun 2021 13:46:08 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17875,"web_url":"https://patchwork.libcamera.org/comment/17875/","msgid":"<2c281e19-82df-98e6-a58c-83e917a3be36@ideasonboard.com>","date":"2021-06-28T08:05:10","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 24/06/2021 14:46, Kieran Bingham wrote:\n> Hi JM,\n> \n> On 22/06/2021 10:45, Jean-Michel Hautbois wrote:\n>> For various sensor operations, it may be needed to do sensor specific\n>> computations, like analogue gain or vertical blanking.\n>>\n>> This commit introduces a new camera sensor helper in libipa which aims\n>> to solve this specific issue.\n>> It is based on the MIPI alliance Specification for Camera Command Set\n>> and implements, for now, only the analogue \"Global gain\" mode.\n>> Setting analogue gain for a specific sensor is not a straightforward\n>> operation, as one needs to know how the gain is calculated for it.\n>>\n>> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>>\n>> Adding a new sensor is pretty straightforward as one only needs to\n>> implement the sub-class for it and register that class to the\n>> CameraSensorHelperFactory.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> Reviewed-by: Jacopo Mondi \n>> ---\n>> src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n>> src/ipa/libipa/camera_sensor_helper.h | 87 +++++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 407 insertions(+)\n>> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n>> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>> new file mode 100644\n>> index 00000000..a114ee73\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -0,0 +1,318 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n>> + */\n>> +#include \"camera_sensor_helper.h\"\n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +/**\n>> + * \\file camera_sensor_helper.h\n>> + * \\brief Helper class that performs sensor-specific parameter computations\n>> + *\n>> + * Computation of sensor configuration parameters is a sensor specific\n>> + * operation. Each CameraHelper derived class computes the value of\n>> + * configuration parameters, for example the analogue gain value, using\n>> + * sensor-specific functions and constants.\n>> + *\n>> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n>> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\class CameraSensorHelper\n>> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n>> + *\n>> + * CameraSensorHelper derived class instances are sensor specific. Each\n>> + * supported sensor will have an associated base class defined.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a CameraSensorHelper instance\n>> + *\n>> + * CameraSensorHelper derived class instances shall never be constructed manually\n>> + * but always through the CameraSensorHelperFactory::create() method.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Compute gain code from the analogue gain absolute value\n>> + * \\param[in] gain The real gain to pass\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + *\n>> + * \\return The gain code to pass to V4l2\n>> + */\n>> +uint32_t CameraSensorHelper::gainCode(double gain) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> \n> This doesn't need brackets around the two comparisons.\n> \n> The == takes precedence over the || so those are evaluated before the\n> logical OR.\n> \n> https://en.cppreference.com/w/cpp/language/operator_precedence\n> \n> I personally don't mind over-scoping though, as it helps make sure\n> things are clear when order of precedence isn't always in peoples minds.\n> \n\nI like it that way to ease reading :-).\n\n> \n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n>> + * \\param[in] gainCode The V4l2 subdev control gain\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations. It is the counterpart of the function\n>> + * CameraSensorHelper::gainCode.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + *\n>> + * \\return The real gain\n>> + */\n>> +double CameraSensorHelper::gain(uint32_t gainCode) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> \n> Same,\n> \n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n>> +\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> \n> We're at v6, so I assume those calculations have been verified by now?\n> \n> In particular all the type promotions ...\n> \n\nWhich are a pain... :-p but in our case it should be ok as far as I can\ntell...\nWould you prefer to see explicit conversions before the return ?\n\n> \n>> +}\n>> +\n>> +/**\n>> + * \\enum CameraSensorHelper::AnalogueGainType\n>> + * \\brief the gain calculation modes as defined by the MIPI CCS\n>> + *\n>> + * Describes the image sensor analogue gain capabilities.\n>> + * Two modes are possible, depending on the sensor: Global and Alternate.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainLinear\n>> + * \\brief Gain is computed using linear gain estimation\n>> + *\n>> + * The relationship between the integer gain parameter and the resulting gain\n>> + * multiplier is given by the following equation:\n>> + *\n>> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>> + *\n>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>> + * image-sensor-specific constants of the sensor.\n>> + * These constants are static parameters, and for any given image sensor either\n>> + * m0 or m1 shall be zero.\n>> + *\n>> + * The full Gain equation therefore reduces to either:\n>> + *\n>> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainExponential\n>> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n>> + *\n>> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n>> + * If the image sensor supports it, then the global analogue gain can be controlled\n>> + * by linear and exponential gain formula:\n>> + *\n>> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n>> + * \\todo not implemented in libipa\n>> + */\n>> +\n>> +/**\n>> + * \\struct CameraSensorHelper::AnalogueGainConstants\n>> + * \\brief Analogue gain constants used for gain calculation\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n>> + * \\brief Analogue gain calculation mode\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n>> + * \\brief Constant used in the analogue Gain coding/decoding\n>> + *\n>> + * \\note either m0 or m1 shall be zero.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + *\n>> + * \\note either m0 or m1 shall be zero.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants_\n>> + * \\brief The analogue gain parameters used for calculation\n>> + *\n>> + * The analogue gain is calculated through a formula, and its parameters are\n>> + * sensor specific. Use this variable to store the values at init time.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperFactory\n>> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n>> + *\n>> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n>> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n>> + * classes. Each CameraSensorHelper subclass shall register itself using the\n>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n>> + * instance of a CameraSensorHelperFactory subclass and register it with the\n>> + * static list of factories.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a camera sensor helper factory\n>> + * \\param[in] name Name of the camera sensor helper class\n>> + *\n>> + * Creating an instance of the factory registers it with the global list of\n>> + * factories, accessible through the factories() function.\n>> + *\n>> + * The factory \\a name is used for debug purpose and shall be unique.\n>> + */\n>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n>> +\t: name_(name)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + * \\param[in] name Name of the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n>> + * corresponding to the factory\n>> + */\n>> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tCameraSensorHelperFactory::factories();\n>> +\n>> +\tfor (CameraSensorHelperFactory *factory : factories) {\n>> +\t\tif (name != factory->name_)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tCameraSensorHelper *helper = factory->createInstance();\n>> +\t\treturn std::unique_ptr(helper);\n>> +\t}\n>> +\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Add a camera sensor helper class to the registry\n>> + * \\param[in] factory Factory to use to construct the camera sensor helper\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n>> + * name.\n>> + */\n>> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n>> +{\n>> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n>> +\n>> +\tfactories.push_back(factory);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all camera sensor helper factories\n>> + *\n>> + * The static factories map is defined inside the function to ensures it gets\n>> + * initialized on first use, without any dependency on link order.\n>> + *\n>> + * \\return The list of camera sensor helper factories\n>> + */\n>> +std::vector &CameraSensorHelperFactory::factories()\n>> +{\n>> +\tstatic std::vector factories;\n>> +\treturn factories;\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperFactory::createInstance()\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + *\n>> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n>> + * macro. It creates a camera sensor helper instance associated with the camera\n>> + * sensor model.\n>> + *\n>> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n>> + * subclass corresponding to the factory\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelperFactory::name_\n>> + * \\brief The name of the factory\n>> + */\n>> +\n>> +/**\n>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n>> + * \\param[in] name Sensor model name used to register the class\n>> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n>> + *\n>> + * Register a CameraSensorHelper subclass with the factory and make it available to\n>> + * try and match devices.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperImx219\n>> + * \\brief Create and give helpers for the imx219 sensor\n>> + */\n>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperImx219()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> \n> If these will grow, should we have one file per camera sensor helper to\n> split these out ?\n> \n> Or is that overkill for the moment? I guess it can be broken out later\n> when new features are added that would make the scale increase.\n> \n> \n> Other than that, I don't want to block this series for things that can\n> continue in development so:\n> \n> Reviewed-by: Kieran Bingham \n> \n> and I'll let you decide if it should be split now or later.\n> \n\nThank you... I won't split it now, but it will be in a short notice\nprobably ;-).\n\n> --\n> Kieran\n> \n> \n> \n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5670\n>> + * \\brief Create and give helpers for the ov5670 sensor\n>> + */\n>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5670()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5693\n>> + * \\brief Create and give helpers for the ov5693 sensor\n>> + */\n>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5693()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n>> new file mode 100644\n>> index 00000000..c43e4bf6\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>> @@ -0,0 +1,87 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelper() = default;\n>> +\tvirtual ~CameraSensorHelper() = default;\n>> +\n>> +\tvirtual uint32_t gainCode(double gain) const;\n>> +\tvirtual double gain(uint32_t gainCode) const;\n>> +\n>> +protected:\n>> +\tenum AnalogueGainType {\n>> +\t\tAnalogueGainLinear = 0,\n>> +\t\tAnalogueGainExponential = 2,\n>> +\t};\n>> +\n>> +\tstruct AnalogueGainConstants {\n>> +\t\tAnalogueGainType type;\n>> +\t\tint16_t m0;\n>> +\t\tint16_t c0;\n>> +\t\tint16_t m1;\n>> +\t\tint16_t c1;\n>> +\t};\n>> +\n>> +\tAnalogueGainConstants analogueGainConstants_;\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>> +};\n>> +\n>> +class CameraSensorHelperFactory\n>> +{\n>> +public:\n>> +\tCameraSensorHelperFactory(const std::string name);\n>> +\tvirtual ~CameraSensorHelperFactory() = default;\n>> +\n>> +\tstatic std::unique_ptr create(const std::string &name);\n>> +\n>> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n>> +\tstatic std::vector &factories();\n>> +\n>> +protected:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>> +\tvirtual CameraSensorHelper *createInstance() = 0;\n>> +\n>> +\tstd::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t \\\n>> +class helper##Factory final : public CameraSensorHelperFactory\t \\\n>> +{ \\\n>> +public: \\\n>> +\thelper##Factory() : CameraSensorHelperFactory(name) {}\t \\\n>> +\t\t\t\t\t\t\t\t \\\n>> +private: \\\n>> +\tCameraSensorHelper *createInstance()\t\t\t \\\n>> +\t{\t\t\t\t\t\t\t \\\n>> +\t\treturn new helper();\t\t\t\t \\\n>> +\t}\t\t\t\t\t\t\t \\\n>> +};\t\t\t\t\t\t\t\t \\\n>> +static helper##Factory global_##helper##Factory;\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 038fc490..dc90542c 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -2,11 +2,13 @@\n>> \n>> libipa_headers = files([\n>> 'algorithm.h',\n>> + 'camera_sensor_helper.h',\n>> 'histogram.h'\n>> ])\n>> \n>> libipa_sources = files([\n>> 'algorithm.cpp',\n>> + 'camera_sensor_helper.cpp',\n>> 'histogram.cpp'\n>> ])\n>> \n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 18A31C3220\n\tfor ;\n\tMon, 28 Jun 2021 08:05:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E47E684D6;\n\tMon, 28 Jun 2021 10:05:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03C70684D2\n\tfor ;\n\tMon, 28 Jun 2021 10:05:13 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:c3ad:78d0:405e:fc33])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87897B8A;\n\tMon, 28 Jun 2021 10:05:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Kiuwn1uM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624867513;\n\tbh=2DlDFBMFZYtmBYX3YLHDCC24TGHO81yJeWOzMshiMuc=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Kiuwn1uMxOKwedOCoR6xT0ZuxbiIshq5IaA5s0r/Cs7JM8+BxXf+Bltc9PAd2ebT9\n\trLrw/kiUFbSVBAyakA1T0qv+MLvONIFpvfImI+dR6lKLlcgrqHbTcLDl6ZX/dAYcx2\n\tAkZuJSJLwSYEIOQqLHvOAN1ezycc7AxOlMCaQAzQ=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<2c281e19-82df-98e6-a58c-83e917a3be36@ideasonboard.com>","Date":"Mon, 28 Jun 2021 10:05:10 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17876,"web_url":"https://patchwork.libcamera.org/comment/17876/","msgid":"<8b3b8193-5b2e-1ddf-0341-336e3a080e38@ideasonboard.com>","date":"2021-06-28T08:15:40","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nOn 24/06/2021 13:45, Umang Jain wrote:\n> HI JM,\n> \n> few minors below:\n> \n> On 6/22/21 3:15 PM, Jean-Michel Hautbois wrote:\n>> For various sensor operations, it may be needed to do sensor specific\n>> computations, like analogue gain or vertical blanking.\n>>\n>> This commit introduces a new camera sensor helper in libipa which aims\n>> to solve this specific issue.\n>> It is based on the MIPI alliance Specification for Camera Command Set\n>> and implements, for now, only the analogue \"Global gain\" mode.\n>> Setting analogue gain for a specific sensor is not a straightforward\n>> operation, as one needs to know how the gain is calculated for it.\n>>\n>> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>>\n>> Adding a new sensor is pretty straightforward as one only needs to\n>> implement the sub-class for it and register that class to the\n>> CameraSensorHelperFactory.\n>>\n>> Signed-off-by: Jean-Michel Hautbois\n>> \n>> Reviewed-by: Jacopo Mondi \n>> ---\n>>   src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n>>   src/ipa/libipa/camera_sensor_helper.h   |  87 +++++++\n>>   src/ipa/libipa/meson.build              |   2 +\n>>   3 files changed, 407 insertions(+)\n>>   create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n>>   create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n>> b/src/ipa/libipa/camera_sensor_helper.cpp\n>> new file mode 100644\n>> index 00000000..a114ee73\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -0,0 +1,318 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.cpp - Helper class that performs\n>> sensor-specific parameter computations\n>> + */\n>> +#include \"camera_sensor_helper.h\"\n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +/**\n>> + * \\file camera_sensor_helper.h\n>> + * \\brief Helper class that performs sensor-specific parameter\n>> computations\n>> + *\n>> + * Computation of sensor configuration parameters is a sensor specific\n>> + * operation. Each CameraHelper derived class computes the value of\n>> + * configuration parameters, for example the analogue gain value, using\n>> + * sensor-specific functions and constants.\n>> + *\n>> + * Every subclass of CameraSensorHelper shall be registered with\n>> libipa using\n>> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n> \n> +1\n> \n> this is nice and how each CameraSensorHelper is registered down below.\n> The usage of this macro is apt.\n> \n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\class CameraSensorHelper\n>> + * \\brief Base class for computing sensor tuning parameters using\n>> sensor-specific constants\n>> + *\n>> + * CameraSensorHelper derived class instances are sensor specific. Each\n> \n> minor: I would write this as for clarity:\n> \n>   * Instances derived from CameraSensorHelper class are sensor specific.\n> Each\n> \n>> + * supported sensor will have an associated base class defined.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a CameraSensorHelper instance\n>> + *\n>> + * CameraSensorHelper derived class instances shall never be\n>> constructed manually\n>> + * but always through the CameraSensorHelperFactory::create() method.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Compute gain code from the analogue gain absolute value\n>> + * \\param[in] gain The real gain to pass\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting\n>> the IPA\n>> + * use the real gain for its estimations.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + *\n>> + * \\return The gain code to pass to V4l2\n>> + */\n>> +uint32_t CameraSensorHelper::gainCode(double gain) const\n>> +{\n>> +    ASSERT((analogueGainConstants_.m0 == 0) ||\n>> (analogueGainConstants_.m1 == 0));\n>> +    ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +    return (analogueGainConstants_.c0 - analogueGainConstants_.c1 *\n>> gain) /\n>> +           (analogueGainConstants_.m1 * gain -\n>> analogueGainConstants_.m0);\n> \n> This won't yield a floating value, ever? In case, it does, are we OK\n> returning it as uint32_t with loss of precision. I am not sure, I\n> haven't read the spec or understand this (maybe it's right, since it's\n> gain-\"code\")\n> \n> \n\nI don't think so, but I can have intermediate values if it scares you :-).\nThe gain is a integer on kernel side, the loss of precision results in a\nminor difference AFAIK.\n\n>> +}\n>> +\n>> +/**\n>> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n>> + * \\param[in] gainCode The V4l2 subdev control gain\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting\n>> the IPA\n>> + * use the real gain for its estimations. It is the counterpart of\n>> the function\n>> + * CameraSensorHelper::gainCode.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + *\n>> + * \\return The real gain\n>> + */\n>> +double CameraSensorHelper::gain(uint32_t gainCode) const\n>> +{\n>> +    ASSERT((analogueGainConstants_.m0 == 0) ||\n>> (analogueGainConstants_.m1 == 0));\n>> +    ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +    return (analogueGainConstants_.m0 * static_cast(gainCode)\n>> + analogueGainConstants_.c0) /\n>> +           (analogueGainConstants_.m1 * static_cast(gainCode)\n>> + analogueGainConstants_.c1);\n>> +}\n>> +\n>> +/**\n>> + * \\enum CameraSensorHelper::AnalogueGainType\n>> + * \\brief the gain calculation modes as defined by the MIPI CCS\n> s/the/The\n>> + *\n>> + * Describes the image sensor analogue gain capabilities.\n>> + * Two modes are possible, depending on the sensor: Global and\n>> Alternate.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainLinear\n>> + * \\brief Gain is computed using linear gain estimation\n>> + *\n>> + * The relationship between the integer gain parameter and the\n>> resulting gain\n>> + * multiplier is given by the following equation:\n>> + *\n>> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>> + *\n>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>> + * image-sensor-specific constants of the sensor.\n>> + * These constants are static parameters, and for any given image\n>> sensor either\n>> + * m0 or m1 shall be zero.\n>> + *\n>> + * The full Gain equation therefore reduces to either:\n>> + *\n>> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainExponential\n>> + * \\brief Gain is computed using exponential gain estimation\n>> (introduced in CCS v1.1)\n>> + *\n>> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also\n>> available.\n>> + * If the image sensor supports it, then the global analogue gain can\n>> be controlled\n>> + * by linear and exponential gain formula:\n>> + *\n>> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n>> + * \\todo not implemented in libipa\n>> + */\n>> +\n>> +/**\n>> + * \\struct CameraSensorHelper::AnalogueGainConstants\n>> + * \\brief Analogue gain constants used for gain calculation\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n>> + * \\brief Analogue gain calculation mode\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n>> + * \\brief Constant used in the analogue Gain coding/decoding\n>> + *\n>> + * \\note either m0 or m1 shall be zero.\n> s/either/Either\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + *\n>> + * \\note either m0 or m1 shall be zero.\n> Same\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants_\n>> + * \\brief The analogue gain parameters used for calculation\n>> + *\n>> + * The analogue gain is calculated through a formula, and its\n>> parameters are\n>> + * sensor specific. Use this variable to store the values at init time.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperFactory\n>> + * \\brief Registration of CameraSensorHelperFactory classes and\n>> creation of instances\n>> + *\n>> + * To facilitate discovery and instantiation of CameraSensorHelper\n>> classes, the\n>> + * CameraSensorHelperFactory class maintains a registry of camera\n>> sensor helper\n>> + * classes. Each CameraSensorHelper subclass shall register itself\n>> using the\n> s/classes./sub-classes. maybe?\n>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a\n>> corresponding\n>> + * instance of a CameraSensorHelperFactory subclass and register it\n>> with the\n>> + * static list of factories.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a camera sensor helper factory\n>> + * \\param[in] name Name of the camera sensor helper class\n>> + *\n>> + * Creating an instance of the factory registers it with the global\n>> list of\n>> + * factories, accessible through the factories() function.\n>> + *\n>> + * The factory \\a name is used for debug purpose and shall be unique.\n>> + */\n>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const\n>> std::string name)\n>> +    : name_(name)\n>> +{\n>> +    registerType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding\n>> to the factory\n>> + * \\param[in] name Name of the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the\n>> CameraSensorHelper subclass\n>> + * corresponding to the factory\n>> + */\n>> +std::unique_ptr\n>> CameraSensorHelperFactory::create(const std::string &name)\n>> +{\n>> +    std::vector &factories =\n>> +        CameraSensorHelperFactory::factories();\n>> +\n>> +    for (CameraSensorHelperFactory *factory : factories) {\n>> +        if (name != factory->name_)\n>> +            continue;\n>> +\n>> +        CameraSensorHelper *helper = factory->createInstance();\n>> +        return std::unique_ptr(helper);\n>> +    }\n>> +\n>> +    return nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Add a camera sensor helper class to the registry\n>> + * \\param[in] factory Factory to use to construct the camera sensor\n>> helper\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the\n>> camera sensor helper\n>> + * name.\n>> + */\n>> +void\n>> CameraSensorHelperFactory::registerType(CameraSensorHelperFactory\n>> *factory)\n>> +{\n>> +    std::vector &factories =\n>> CameraSensorHelperFactory::factories();\n>> +\n>> +    factories.push_back(factory);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all camera sensor helper factories\n>> + *\n>> + * The static factories map is defined inside the function to ensures\n>> it gets\n>> + * initialized on first use, without any dependency on link order.\n>> + *\n>> + * \\return The list of camera sensor helper factories\n>> + */\n>> +std::vector\n>> &CameraSensorHelperFactory::factories()\n>> +{\n>> +    static std::vector factories;\n>> +    return factories;\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperFactory::createInstance()\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding\n>> to the factory\n>> + *\n>> + * This virtual function is implemented by the\n>> REGISTER_CAMERA_SENSOR_HELPER()\n>> + * macro. It creates a camera sensor helper instance associated with\n>> the camera\n>> + * sensor model.\n>> + *\n>> + * \\return A pointer to a newly constructed instance of the\n>> CameraSensorHelper\n>> + * subclass corresponding to the factory\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelperFactory::name_\n>> + * \\brief The name of the factory\n>> + */\n>> +\n>> +/**\n>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>> + * \\brief Register a camera sensor helper with the camera sensor\n>> helper factory\n>> + * \\param[in] name Sensor model name used to register the class\n>> + * \\param[in] helper Class name of CameraSensorHelper derived class\n>> to register\n>> + *\n>> + * Register a CameraSensorHelper subclass with the factory and make\n>> it available to\n>> + * try and match devices.\n> \n> I do wonder about the reasons that registering a camera sensor helper\n> needs to create a derived factory class first\n> and  then, it is responsible to create Instance(s) of that camera sensor\n> helper per se?\n> \n> Can CameraSensorHelper base class be flexible enough to create those\n> instances by itself and keeping the registry internal?\n> I am not sure, there might be some road-blocks I am not able to see\n> right now.\n> \n> .... but this isn't that complicated either :-)\n> \n> So,\n> \n> Reviewed-by: Umang Jain \n\nThanks.\n\nThe way it is designed is based on a Factory design pattern, and mostly\ncomes from PipelineHandler implementation.\nI like it that way and can't see an obvious performance gain to have it\nkeeping the registry. It is called only at init time, so we are not in a\nrush :-).\n\n> \n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperImx219\n>> + * \\brief Create and give helpers for the imx219 sensor\n>> + */\n>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +    CameraSensorHelperImx219()\n>> +    {\n>> +        analogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256,\n>> 256 };\n>> +    }\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5670\n>> + * \\brief Create and give helpers for the ov5670 sensor\n>> + */\n>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +    CameraSensorHelperOv5670()\n>> +    {\n>> +        analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>> +    }\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5693\n>> + * \\brief Create and give helpers for the ov5693 sensor\n>> + */\n>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +    CameraSensorHelperOv5693()\n>> +    {\n>> +        analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>> +    }\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n>> b/src/ipa/libipa/camera_sensor_helper.h\n>> new file mode 100644\n>> index 00000000..c43e4bf6\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>> @@ -0,0 +1,87 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.h - Helper class that performs\n>> sensor-specific parameter computations\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class CameraSensorHelper\n>> +{\n>> +public:\n>> +    CameraSensorHelper() = default;\n>> +    virtual ~CameraSensorHelper() = default;\n>> +\n>> +    virtual uint32_t gainCode(double gain) const;\n>> +    virtual double gain(uint32_t gainCode) const;\n>> +\n>> +protected:\n>> +    enum AnalogueGainType {\n>> +        AnalogueGainLinear = 0,\n>> +        AnalogueGainExponential = 2,\n>> +    };\n>> +\n>> +    struct AnalogueGainConstants {\n>> +        AnalogueGainType type;\n>> +        int16_t m0;\n>> +        int16_t c0;\n>> +        int16_t m1;\n>> +        int16_t c1;\n>> +    };\n>> +\n>> +    AnalogueGainConstants analogueGainConstants_;\n>> +\n>> +private:\n>> +    LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>> +};\n>> +\n>> +class CameraSensorHelperFactory\n>> +{\n>> +public:\n>> +    CameraSensorHelperFactory(const std::string name);\n>> +    virtual ~CameraSensorHelperFactory() = default;\n>> +\n>> +    static std::unique_ptr create(const\n>> std::string &name);\n>> +\n>> +    static void registerType(CameraSensorHelperFactory *factory);\n>> +    static std::vector &factories();\n>> +\n>> +protected:\n>> +    LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>> +    virtual CameraSensorHelper *createInstance() = 0;\n>> +\n>> +    std::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)          \\\n>> +class helper##Factory final : public CameraSensorHelperFactory      \\\n>> +{                                                                 \\\n>> +public:                                                           \\\n>> +    helper##Factory() : CameraSensorHelperFactory(name) {}      \\\n>> +                                  \\\n>> +private:                                                          \\\n>> +    CameraSensorHelper *createInstance()              \\\n>> +    {                              \\\n>> +        return new helper();                  \\\n>> +    }                              \\\n>> +};                                  \\\n>> +static helper##Factory global_##helper##Factory;\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 038fc490..dc90542c 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -2,11 +2,13 @@\n>>     libipa_headers = files([\n>>       'algorithm.h',\n>> +    'camera_sensor_helper.h',\n>>       'histogram.h'\n>>   ])\n>>     libipa_sources = files([\n>>       'algorithm.cpp',\n>> +    'camera_sensor_helper.cpp',\n>>       'histogram.cpp'\n>>   ])\n>>   \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 19E82C321F\n\tfor ;\n\tMon, 28 Jun 2021 08:15:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 895FB684D8;\n\tMon, 28 Jun 2021 10:15:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A86A0684D2\n\tfor ;\n\tMon, 28 Jun 2021 10:15:43 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:c3ad:78d0:405e:fc33])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3471BB8A;\n\tMon, 28 Jun 2021 10:15:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wSvVv/GL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624868143;\n\tbh=u7WHkQ5uZZ4ZeULrQObEFx1iKo+AR7zyq+YTtozQas4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=wSvVv/GLCFWYxDTiFkd07aZqgVZ7hU6rdBClzfBsusl7Xv4oVJ60jJV/n9Yk8uoiY\n\tGLCg/KbFaScmk/MFIpElLQ3tGmT2fIN4BtC77nbYRouFU0fI9fgyfav45Nt4+zfohV\n\tXzPuERpCoSRUMrZR0Kzldxr5Jg0n1Y9GLHnANt/g=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<8b3b8193-5b2e-1ddf-0341-336e3a080e38@ideasonboard.com>","Date":"Mon, 28 Jun 2021 10:15:40 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17882,"web_url":"https://patchwork.libcamera.org/comment/17882/","msgid":"<9ed4f08e-f09b-e317-72ed-9d8a5b7af83a@ideasonboard.com>","date":"2021-06-28T09:16:11","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nOn 6/28/21 1:45 PM, Jean-Michel Hautbois wrote:\n> Hi Umang,\n>\n> On 24/06/2021 13:45, Umang Jain wrote:\n>> HI JM,\n>>\n>> few minors below:\n>>\n>> On 6/22/21 3:15 PM, Jean-Michel Hautbois wrote:\n>>> For various sensor operations, it may be needed to do sensor specific\n>>> computations, like analogue gain or vertical blanking.\n>>>\n>>> This commit introduces a new camera sensor helper in libipa which aims\n>>> to solve this specific issue.\n>>> It is based on the MIPI alliance Specification for Camera Command Set\n>>> and implements, for now, only the analogue \"Global gain\" mode.\n>>> Setting analogue gain for a specific sensor is not a straightforward\n>>> operation, as one needs to know how the gain is calculated for it.\n>>>\n>>> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>>>\n>>> Adding a new sensor is pretty straightforward as one only needs to\n>>> implement the sub-class for it and register that class to the\n>>> CameraSensorHelperFactory.\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois\n>>> \n>>> Reviewed-by: Jacopo Mondi \n>>> ---\n>>>   src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n>>>   src/ipa/libipa/camera_sensor_helper.h   |  87 +++++++\n>>>   src/ipa/libipa/meson.build              |   2 +\n>>>   3 files changed, 407 insertions(+)\n>>>   create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n>>>   create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>>>\n>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n>>> b/src/ipa/libipa/camera_sensor_helper.cpp\n>>> new file mode 100644\n>>> index 00000000..a114ee73\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>>> @@ -0,0 +1,318 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Google Inc.\n>>> + *\n>>> + * camera_sensor_helper.cpp - Helper class that performs\n>>> sensor-specific parameter computations\n>>> + */\n>>> +#include \"camera_sensor_helper.h\"\n>>> +\n>>> +#include \"libcamera/internal/log.h\"\n>>> +\n>>> +/**\n>>> + * \\file camera_sensor_helper.h\n>>> + * \\brief Helper class that performs sensor-specific parameter\n>>> computations\n>>> + *\n>>> + * Computation of sensor configuration parameters is a sensor specific\n>>> + * operation. Each CameraHelper derived class computes the value of\n>>> + * configuration parameters, for example the analogue gain value, using\n>>> + * sensor-specific functions and constants.\n>>> + *\n>>> + * Every subclass of CameraSensorHelper shall be registered with\n>>> libipa using\n>>> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n>> +1\n>>\n>> this is nice and how each CameraSensorHelper is registered down below.\n>> The usage of this macro is apt.\n>>\n>>> + */\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelper\n>>> + * \\brief Base class for computing sensor tuning parameters using\n>>> sensor-specific constants\n>>> + *\n>>> + * CameraSensorHelper derived class instances are sensor specific. Each\n>> minor: I would write this as for clarity:\n>>\n>>   * Instances derived from CameraSensorHelper class are sensor specific.\n>> Each\n>>\n>>> + * supported sensor will have an associated base class defined.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct a CameraSensorHelper instance\n>>> + *\n>>> + * CameraSensorHelper derived class instances shall never be\n>>> constructed manually\n>>> + * but always through the CameraSensorHelperFactory::create() method.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Compute gain code from the analogue gain absolute value\n>>> + * \\param[in] gain The real gain to pass\n>>> + *\n>>> + * This function aims to abstract the calculation of the gain letting\n>>> the IPA\n>>> + * use the real gain for its estimations.\n>>> + *\n>>> + * The parameters come from the MIPI Alliance Camera Specification for\n>>> + * Camera Command Set (CCS).\n>>> + *\n>>> + * \\return The gain code to pass to V4l2\n>>> + */\n>>> +uint32_t CameraSensorHelper::gainCode(double gain) const\n>>> +{\n>>> +    ASSERT((analogueGainConstants_.m0 == 0) ||\n>>> (analogueGainConstants_.m1 == 0));\n>>> +    ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>>> +\n>>> +    return (analogueGainConstants_.c0 - analogueGainConstants_.c1 *\n>>> gain) /\n>>> +           (analogueGainConstants_.m1 * gain -\n>>> analogueGainConstants_.m0);\n>> This won't yield a floating value, ever? In case, it does, are we OK\n>> returning it as uint32_t with loss of precision. I am not sure, I\n>> haven't read the spec or understand this (maybe it's right, since it's\n>> gain-\"code\")\n>>\n>>\n> I don't think so, but I can have intermediate values if it scares you :-).\n> The gain is a integer on kernel side, the loss of precision results in a\n> minor difference AFAIK.\n\n\nhmm, in that case, I think you can capture this context as a small-ish \ncomment here, describing that, one can except intermediate values on \ndivision but gain is an integer on kernel side...\n\n>\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n>>> + * \\param[in] gainCode The V4l2 subdev control gain\n>>> + *\n>>> + * This function aims to abstract the calculation of the gain letting\n>>> the IPA\n>>> + * use the real gain for its estimations. It is the counterpart of\n>>> the function\n>>> + * CameraSensorHelper::gainCode.\n>>> + *\n>>> + * The parameters come from the MIPI Alliance Camera Specification for\n>>> + * Camera Command Set (CCS).\n>>> + *\n>>> + * \\return The real gain\n>>> + */\n>>> +double CameraSensorHelper::gain(uint32_t gainCode) const\n>>> +{\n>>> +    ASSERT((analogueGainConstants_.m0 == 0) ||\n>>> (analogueGainConstants_.m1 == 0));\n>>> +    ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>>> +\n>>> +    return (analogueGainConstants_.m0 * static_cast(gainCode)\n>>> + analogueGainConstants_.c0) /\n>>> +           (analogueGainConstants_.m1 * static_cast(gainCode)\n>>> + analogueGainConstants_.c1);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\enum CameraSensorHelper::AnalogueGainType\n>>> + * \\brief the gain calculation modes as defined by the MIPI CCS\n>> s/the/The\n>>> + *\n>>> + * Describes the image sensor analogue gain capabilities.\n>>> + * Two modes are possible, depending on the sensor: Global and\n>>> Alternate.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainLinear\n>>> + * \\brief Gain is computed using linear gain estimation\n>>> + *\n>>> + * The relationship between the integer gain parameter and the\n>>> resulting gain\n>>> + * multiplier is given by the following equation:\n>>> + *\n>>> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>>> + *\n>>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>>> + * image-sensor-specific constants of the sensor.\n>>> + * These constants are static parameters, and for any given image\n>>> sensor either\n>>> + * m0 or m1 shall be zero.\n>>> + *\n>>> + * The full Gain equation therefore reduces to either:\n>>> + *\n>>> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainExponential\n>>> + * \\brief Gain is computed using exponential gain estimation\n>>> (introduced in CCS v1.1)\n>>> + *\n>>> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also\n>>> available.\n>>> + * If the image sensor supports it, then the global analogue gain can\n>>> be controlled\n>>> + * by linear and exponential gain formula:\n>>> + *\n>>> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n>>> + * \\todo not implemented in libipa\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\struct CameraSensorHelper::AnalogueGainConstants\n>>> + * \\brief Analogue gain constants used for gain calculation\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n>>> + * \\brief Analogue gain calculation mode\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n>>> + * \\brief Constant used in the analogue Gain coding/decoding\n>>> + *\n>>> + * \\note either m0 or m1 shall be zero.\n>> s/either/Either\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n>>> + * \\brief Constant used in the analogue gain coding/decoding\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n>>> + * \\brief Constant used in the analogue gain coding/decoding\n>>> + *\n>>> + * \\note either m0 or m1 shall be zero.\n>> Same\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n>>> + * \\brief Constant used in the analogue gain coding/decoding\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::analogueGainConstants_\n>>> + * \\brief The analogue gain parameters used for calculation\n>>> + *\n>>> + * The analogue gain is calculated through a formula, and its\n>>> parameters are\n>>> + * sensor specific. Use this variable to store the values at init time.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperFactory\n>>> + * \\brief Registration of CameraSensorHelperFactory classes and\n>>> creation of instances\n>>> + *\n>>> + * To facilitate discovery and instantiation of CameraSensorHelper\n>>> classes, the\n>>> + * CameraSensorHelperFactory class maintains a registry of camera\n>>> sensor helper\n>>> + * classes. Each CameraSensorHelper subclass shall register itself\n>>> using the\n>> s/classes./sub-classes. maybe?\n>>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a\n>>> corresponding\n>>> + * instance of a CameraSensorHelperFactory subclass and register it\n>>> with the\n>>> + * static list of factories.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct a camera sensor helper factory\n>>> + * \\param[in] name Name of the camera sensor helper class\n>>> + *\n>>> + * Creating an instance of the factory registers it with the global\n>>> list of\n>>> + * factories, accessible through the factories() function.\n>>> + *\n>>> + * The factory \\a name is used for debug purpose and shall be unique.\n>>> + */\n>>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const\n>>> std::string name)\n>>> +    : name_(name)\n>>> +{\n>>> +    registerType(this);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Create an instance of the CameraSensorHelper corresponding\n>>> to the factory\n>>> + * \\param[in] name Name of the factory\n>>> + *\n>>> + * \\return A unique pointer to a new instance of the\n>>> CameraSensorHelper subclass\n>>> + * corresponding to the factory\n>>> + */\n>>> +std::unique_ptr\n>>> CameraSensorHelperFactory::create(const std::string &name)\n>>> +{\n>>> +    std::vector &factories =\n>>> +        CameraSensorHelperFactory::factories();\n>>> +\n>>> +    for (CameraSensorHelperFactory *factory : factories) {\n>>> +        if (name != factory->name_)\n>>> +            continue;\n>>> +\n>>> +        CameraSensorHelper *helper = factory->createInstance();\n>>> +        return std::unique_ptr(helper);\n>>> +    }\n>>> +\n>>> +    return nullptr;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Add a camera sensor helper class to the registry\n>>> + * \\param[in] factory Factory to use to construct the camera sensor\n>>> helper\n>>> + *\n>>> + * The caller is responsible to guarantee the uniqueness of the\n>>> camera sensor helper\n>>> + * name.\n>>> + */\n>>> +void\n>>> CameraSensorHelperFactory::registerType(CameraSensorHelperFactory\n>>> *factory)\n>>> +{\n>>> +    std::vector &factories =\n>>> CameraSensorHelperFactory::factories();\n>>> +\n>>> +    factories.push_back(factory);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Retrieve the list of all camera sensor helper factories\n>>> + *\n>>> + * The static factories map is defined inside the function to ensures\n>>> it gets\n>>> + * initialized on first use, without any dependency on link order.\n>>> + *\n>>> + * \\return The list of camera sensor helper factories\n>>> + */\n>>> +std::vector\n>>> &CameraSensorHelperFactory::factories()\n>>> +{\n>>> +    static std::vector factories;\n>>> +    return factories;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\fn CameraSensorHelperFactory::createInstance()\n>>> + * \\brief Create an instance of the CameraSensorHelper corresponding\n>>> to the factory\n>>> + *\n>>> + * This virtual function is implemented by the\n>>> REGISTER_CAMERA_SENSOR_HELPER()\n>>> + * macro. It creates a camera sensor helper instance associated with\n>>> the camera\n>>> + * sensor model.\n>>> + *\n>>> + * \\return A pointer to a newly constructed instance of the\n>>> CameraSensorHelper\n>>> + * subclass corresponding to the factory\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelperFactory::name_\n>>> + * \\brief The name of the factory\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>>> + * \\brief Register a camera sensor helper with the camera sensor\n>>> helper factory\n>>> + * \\param[in] name Sensor model name used to register the class\n>>> + * \\param[in] helper Class name of CameraSensorHelper derived class\n>>> to register\n>>> + *\n>>> + * Register a CameraSensorHelper subclass with the factory and make\n>>> it available to\n>>> + * try and match devices.\n>> I do wonder about the reasons that registering a camera sensor helper\n>> needs to create a derived factory class first\n>> and  then, it is responsible to create Instance(s) of that camera sensor\n>> helper per se?\n>>\n>> Can CameraSensorHelper base class be flexible enough to create those\n>> instances by itself and keeping the registry internal?\n>> I am not sure, there might be some road-blocks I am not able to see\n>> right now.\n>>\n>> .... but this isn't that complicated either :-)\n>>\n>> So,\n>>\n>> Reviewed-by: Umang Jain \n> Thanks.\n>\n> The way it is designed is based on a Factory design pattern, and mostly\n> comes from PipelineHandler implementation.\n> I like it that way and can't see an obvious performance gain to have it\n> keeping the registry. It is called only at init time, so we are not in a\n> rush :-).\n>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperImx219\n>>> + * \\brief Create and give helpers for the imx219 sensor\n>>> + */\n>>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +    CameraSensorHelperImx219()\n>>> +    {\n>>> +        analogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256,\n>>> 256 };\n>>> +    }\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperOv5670\n>>> + * \\brief Create and give helpers for the ov5670 sensor\n>>> + */\n>>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +    CameraSensorHelperOv5670()\n>>> +    {\n>>> +        analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>>> +    }\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperOv5693\n>>> + * \\brief Create and give helpers for the ov5693 sensor\n>>> + */\n>>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +    CameraSensorHelperOv5693()\n>>> +    {\n>>> +        analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>>> +    }\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n>>> b/src/ipa/libipa/camera_sensor_helper.h\n>>> new file mode 100644\n>>> index 00000000..c43e4bf6\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>>> @@ -0,0 +1,87 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Google Inc.\n>>> + *\n>>> + * camera_sensor_helper.h - Helper class that performs\n>>> sensor-specific parameter computations\n>>> + */\n>>> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>>> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>>> +\n>>> +#include \n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +class CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +    CameraSensorHelper() = default;\n>>> +    virtual ~CameraSensorHelper() = default;\n>>> +\n>>> +    virtual uint32_t gainCode(double gain) const;\n>>> +    virtual double gain(uint32_t gainCode) const;\n>>> +\n>>> +protected:\n>>> +    enum AnalogueGainType {\n>>> +        AnalogueGainLinear = 0,\n>>> +        AnalogueGainExponential = 2,\n>>> +    };\n>>> +\n>>> +    struct AnalogueGainConstants {\n>>> +        AnalogueGainType type;\n>>> +        int16_t m0;\n>>> +        int16_t c0;\n>>> +        int16_t m1;\n>>> +        int16_t c1;\n>>> +    };\n>>> +\n>>> +    AnalogueGainConstants analogueGainConstants_;\n>>> +\n>>> +private:\n>>> +    LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>>> +};\n>>> +\n>>> +class CameraSensorHelperFactory\n>>> +{\n>>> +public:\n>>> +    CameraSensorHelperFactory(const std::string name);\n>>> +    virtual ~CameraSensorHelperFactory() = default;\n>>> +\n>>> +    static std::unique_ptr create(const\n>>> std::string &name);\n>>> +\n>>> +    static void registerType(CameraSensorHelperFactory *factory);\n>>> +    static std::vector &factories();\n>>> +\n>>> +protected:\n>>> +    LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>>> +    virtual CameraSensorHelper *createInstance() = 0;\n>>> +\n>>> +    std::string name_;\n>>> +};\n>>> +\n>>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)          \\\n>>> +class helper##Factory final : public CameraSensorHelperFactory      \\\n>>> +{                                                                 \\\n>>> +public:                                                           \\\n>>> +    helper##Factory() : CameraSensorHelperFactory(name) {}      \\\n>>> +                                  \\\n>>> +private:                                                          \\\n>>> +    CameraSensorHelper *createInstance()              \\\n>>> +    {                              \\\n>>> +        return new helper();                  \\\n>>> +    }                              \\\n>>> +};                                  \\\n>>> +static helper##Factory global_##helper##Factory;\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>> index 038fc490..dc90542c 100644\n>>> --- a/src/ipa/libipa/meson.build\n>>> +++ b/src/ipa/libipa/meson.build\n>>> @@ -2,11 +2,13 @@\n>>>     libipa_headers = files([\n>>>       'algorithm.h',\n>>> +    'camera_sensor_helper.h',\n>>>       'histogram.h'\n>>>   ])\n>>>     libipa_sources = files([\n>>>       'algorithm.cpp',\n>>> +    'camera_sensor_helper.cpp',\n>>>       'histogram.cpp'\n>>>   ])\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 604BFC321F\n\tfor ;\n\tMon, 28 Jun 2021 09:16:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E6C1684D8;\n\tMon, 28 Jun 2021 11:16:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F26B3684D2\n\tfor ;\n\tMon, 28 Jun 2021 11:16:15 +0200 (CEST)","from [192.168.1.108] (unknown [103.74.72.2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC88BB8A;\n\tMon, 28 Jun 2021 11:16:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BAHIEg+H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624871775;\n\tbh=tDzlYHEOfS11goAZUqk0nIBvvveXb8UIQZ0a8b/D2UE=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=BAHIEg+HW5+1vt+lpRUmR7oLDrcqw9skYv8O9rI3FrTLusPae3TVm55Y1KfZg1bdm\n\tgrdlzyqoz7cohpEqL1fIKNto28GfiIOdTAlXicYhLt8U0DQ7TYEw/ugZtO3JPh7fDL\n\t0kGgoL6j8DlDvJ7r25FXaw1+itM0SJdZsdGrWXDI=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<8b3b8193-5b2e-1ddf-0341-336e3a080e38@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<9ed4f08e-f09b-e317-72ed-9d8a5b7af83a@ideasonboard.com>","Date":"Mon, 28 Jun 2021 14:46:11 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<8b3b8193-5b2e-1ddf-0341-336e3a080e38@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17896,"web_url":"https://patchwork.libcamera.org/comment/17896/","msgid":"","date":"2021-06-28T16:18:44","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nI know this has been merged already, I should have reviewed this series\nsooner. Could you please address the issues pointed out below in a new\nseries ?\n\nOn Tue, Jun 22, 2021 at 11:45:22AM +0200, Jean-Michel Hautbois wrote:\n> For various sensor operations, it may be needed to do sensor specific\n> computations, like analogue gain or vertical blanking.\n> \n> This commit introduces a new camera sensor helper in libipa which aims\n> to solve this specific issue.\n> It is based on the MIPI alliance Specification for Camera Command Set\n> and implements, for now, only the analogue \"Global gain\" mode.\n> Setting analogue gain for a specific sensor is not a straightforward\n> operation, as one needs to know how the gain is calculated for it.\n> \n> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n> \n> Adding a new sensor is pretty straightforward as one only needs to\n> implement the sub-class for it and register that class to the\n> CameraSensorHelperFactory.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 87 +++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 407 insertions(+)\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..a114ee73\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,318 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n\nLine break at 80 columns ? There are some long lines below that can be\neasily broken at the 80 columns limit too (such as the \\brief for the\nCameraSensorClass for instance).\n\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n\nThis has turned into \"libcamera/base/log.h\" but it should be\n.\n\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Helper class that performs sensor-specific parameter computations\n> + *\n> + * Computation of sensor configuration parameters is a sensor specific\n\ns/sensor specific/sensor-specific/\n\n> + * operation. Each CameraHelper derived class computes the value of\n> + * configuration parameters, for example the analogue gain value, using\n> + * sensor-specific functions and constants.\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class CameraSensorHelper\n> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n> + *\n> + * CameraSensorHelper derived class instances are sensor specific. Each\n> + * supported sensor will have an associated base class defined.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + *\n> + * CameraSensorHelper derived class instances shall never be constructed manually\n> + * but always through the CameraSensorHelperFactory::create() method.\n> + */\n> +\n> +/**\n> + * \\brief Compute gain code from the analogue gain absolute value\n> + * \\param[in] gain The real gain to pass\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The gain code to pass to V4l2\n\ns/V4l2/V4L2/\n\nSame below.\n\nIt's however not very nice to make this aware of V4L2. I'll propose a\npatch to improve the documentation, which will also define the concept\nof gain code explicitly.\n\n> + */\n> +uint32_t CameraSensorHelper::gainCode(double gain) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n\nNo need for the inner parentheses. Same below.\n\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +}\n> +\n> +/**\n> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of the function\n> + * CameraSensorHelper::gainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The real gain\n> + */\n> +double CameraSensorHelper::gain(uint32_t gainCode) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> +\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> +}\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainType\n> + * \\brief the gain calculation modes as defined by the MIPI CCS\n> + *\n> + * Describes the image sensor analogue gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n\nThey're called Linear and Exponential here. Mentioning CCS for reference\nis fine, but this is our implementation, with our names.\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainLinear\n> + * \\brief Gain is computed using linear gain estimation\n> + *\n> + * The relationship between the integer gain parameter and the resulting gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> + *\n> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> + * image-sensor-specific constants of the sensor.\n> + * These constants are static parameters, and for any given image sensor either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainExponential\n> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n> + * If the image sensor supports it, then the global analogue gain can be controlled\n> + * by linear and exponential gain formula:\n> + *\n> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n> + * \\todo not implemented in libipa\n> + */\n> +\n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainConstants\n> + * \\brief Analogue gain constants used for gain calculation\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n> + * \\brief Analogue gain calculation mode\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> + * \\brief Constant used in the analogue Gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n\ns/the factory/a named factory/\n\nPipelineHandlerFactory::create() is not a static function, so it's\ndifferent.\n\n> + * \\param[in] name Name of the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> + * corresponding to the factory\n\ns/the factory/the named factory, or a null pointer if no such factory exists/\n\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +{\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactory::factories();\n> +\n> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> +\t\tif (name != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> +\t\treturn std::unique_ptr(helper);\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor helper class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +{\n> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n\nLine wrap.\n\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor helper factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n\nI know this comes from pipeline_handler.cpp, but this doesn't belong to\nthe API documentation, it's an implementation detail. You can move this\ncomment inside the function. Bonus points if you also fix the issue in\npipeline_handler.cpp.\n\n> + *\n> + * \\return The list of camera sensor helper factories\n> + */\n> +std::vector &CameraSensorHelperFactory::factories()\n> +{\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelperFactory::name_\n> + * \\brief The name of the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> + * try and match devices.\n\ns/devices/sensors/\n\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperImx219\n> + * \\brief Create and give helpers for the imx219 sensor\n> + */\n\nYou can drop the comments for the subclasses, they don't bring any\nvalue. If doxygen complains, you can enclose all the subclasses with a\n\n#ifndef __DOXYGEN__\n...\n#endif /* __DOXYGEN__ */\n\nYou could also add a\n\n/* -----------------------------------------------------------------------------\n * Sensor-speficic subclasses\n */\n\ncomment here if you want to separate it from the generic code above.\n\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..c43e4bf6\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelper() = default;\n> +\tvirtual ~CameraSensorHelper() = default;\n> +\n> +\tvirtual uint32_t gainCode(double gain) const;\n> +\tvirtual double gain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainType {\n> +\t\tAnalogueGainLinear = 0,\n> +\t\tAnalogueGainExponential = 2,\n\nWhy 2 and not 1 ? I'd actually drop the numerical values completely.\n\n> +\t};\n> +\n> +\tstruct AnalogueGainConstants {\n> +\t\tAnalogueGainType type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\n> +\tAnalogueGainConstants analogueGainConstants_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(const std::string &name);\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +protected:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n\nThis should be private.\n\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n\nI think this can be private too.\n\nprotected:\n\tvirtual CameraSensorHelper *createInstance() = 0;\n\nprivate:\n\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n\n\tstd::string name_;\n\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t \\\n> +class helper##Factory final : public CameraSensorHelperFactory\t \\\n> +{ \\\n\nWe use tabs for indentation.\n\n> +public: \\\n> +\thelper##Factory() : CameraSensorHelperFactory(name) {}\t \\\n> +\t\t\t\t\t\t\t\t \\\n> +private: \\\n> +\tCameraSensorHelper *createInstance()\t\t\t \\\n> +\t{\t\t\t\t\t\t\t \\\n> +\t\treturn new helper();\t\t\t\t \\\n> +\t}\t\t\t\t\t\t\t \\\n> +};\t\t\t\t\t\t\t\t \\\n> +static helper##Factory global_##helper##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 038fc490..dc90542c 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,11 +2,13 @@\n> \n> libipa_headers = files([\n> 'algorithm.h',\n> + 'camera_sensor_helper.h',\n> 'histogram.h'\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'camera_sensor_helper.cpp',\n> 'histogram.cpp'\n> ])\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EB9B7C321F\n\tfor ;\n\tMon, 28 Jun 2021 16:18:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 390C2684D7;\n\tMon, 28 Jun 2021 18:18:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88E1A6028C\n\tfor ;\n\tMon, 28 Jun 2021 18:18:46 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 05C2DB8A;\n\tMon, 28 Jun 2021 18:18:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SXtZTwQv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624897126;\n\tbh=jtw6N9UyTwUUEF2S6K/HK0fjzDqbx0VCyTkEPkpgqS4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SXtZTwQvWZJ+KXBS4yUo63P1EkygGMCGL1qptHjtJacJtlXSRAqgX7IVnT2dT04H+\n\tAs9fL+PPD5cskmzTFZOCbSFwaIp3N1EujRSD1jIV0aOBPUWUcwUl5XS9W6J3+qzdla\n\tYa+7eRr6T88lxP0t1Pn7FNNQ0BZeNCSUZPeSpXjY=","Date":"Mon, 28 Jun 2021 19:18:44 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17911,"web_url":"https://patchwork.libcamera.org/comment/17911/","msgid":"","date":"2021-06-29T12:44:51","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 22/06/2021 10:45, Jean-Michel Hautbois wrote:\n> For various sensor operations, it may be needed to do sensor specific\n> computations, like analogue gain or vertical blanking.\n> \n> This commit introduces a new camera sensor helper in libipa which aims\n> to solve this specific issue.\n> It is based on the MIPI alliance Specification for Camera Command Set\n> and implements, for now, only the analogue \"Global gain\" mode.\n> Setting analogue gain for a specific sensor is not a straightforward\n> operation, as one needs to know how the gain is calculated for it.\n> \n> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n> \n> Adding a new sensor is pretty straightforward as one only needs to\n> implement the sub-class for it and register that class to the\n> CameraSensorHelperFactory.\n\nWhile extending the factories to add a new class seems simple, I'm\nhaving a hard time to determine what the 'correct' way to extend it is.\n\nHow would I determine the correct values for each of the fields for\ninstance?\n\nIs there any guidance on where this information comes from or how it\nshould be determined? or are they values you already had somewhere?\n\nFor instance, we now need to add the ov13858, which is on the Soraka.\n\nWhat should we follow to add that ?\n\nI've added defaults by copying another as an example, but that's no\ndifferent to us supplying incorrect defaults otherwise.\n\nI think outright failure when this data isn't provided is a better\noption here though (which is what has happened to me), as it forces the\nuser to find out how to get the right values, and make sure they are\nadded - but the easier it is to demonstrate how to do that correctly,\nthe better.\n\n--\nKieran\n\n\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 87 +++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 407 insertions(+)\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..a114ee73\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,318 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Helper class that performs sensor-specific parameter computations\n> + *\n> + * Computation of sensor configuration parameters is a sensor specific\n> + * operation. Each CameraHelper derived class computes the value of\n> + * configuration parameters, for example the analogue gain value, using\n> + * sensor-specific functions and constants.\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class CameraSensorHelper\n> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n> + *\n> + * CameraSensorHelper derived class instances are sensor specific. Each\n> + * supported sensor will have an associated base class defined.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + *\n> + * CameraSensorHelper derived class instances shall never be constructed manually\n> + * but always through the CameraSensorHelperFactory::create() method.\n> + */\n> +\n> +/**\n> + * \\brief Compute gain code from the analogue gain absolute value\n> + * \\param[in] gain The real gain to pass\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The gain code to pass to V4l2\n> + */\n> +uint32_t CameraSensorHelper::gainCode(double gain) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +}\n> +\n> +/**\n> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of the function\n> + * CameraSensorHelper::gainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + *\n> + * \\return The real gain\n> + */\n> +double CameraSensorHelper::gain(uint32_t gainCode) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> +\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> +}\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainType\n> + * \\brief the gain calculation modes as defined by the MIPI CCS\n> + *\n> + * Describes the image sensor analogue gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainLinear\n> + * \\brief Gain is computed using linear gain estimation\n> + *\n> + * The relationship between the integer gain parameter and the resulting gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> + *\n> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> + * image-sensor-specific constants of the sensor.\n> + * These constants are static parameters, and for any given image sensor either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainExponential\n> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n> + * If the image sensor supports it, then the global analogue gain can be controlled\n> + * by linear and exponential gain formula:\n> + *\n> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n> + * \\todo not implemented in libipa\n> + */\n> +\n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainConstants\n> + * \\brief Analogue gain constants used for gain calculation\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n> + * \\brief Analogue gain calculation mode\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> + * \\brief Constant used in the analogue Gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + *\n> + * \\note either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + * \\param[in] name Name of the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +{\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactory::factories();\n> +\n> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> +\t\tif (name != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> +\t\treturn std::unique_ptr(helper);\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor helper class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +{\n> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor helper factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return The list of camera sensor helper factories\n> + */\n> +std::vector &CameraSensorHelperFactory::factories()\n> +{\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelperFactory::name_\n> + * \\brief The name of the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> + * try and match devices.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperImx219\n> + * \\brief Create and give helpers for the imx219 sensor\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..c43e4bf6\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelper() = default;\n> +\tvirtual ~CameraSensorHelper() = default;\n> +\n> +\tvirtual uint32_t gainCode(double gain) const;\n> +\tvirtual double gain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainType {\n> +\t\tAnalogueGainLinear = 0,\n> +\t\tAnalogueGainExponential = 2,\n> +\t};\n> +\n> +\tstruct AnalogueGainConstants {\n> +\t\tAnalogueGainType type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\n> +\tAnalogueGainConstants analogueGainConstants_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(const std::string &name);\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +protected:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t \\\n> +class helper##Factory final : public CameraSensorHelperFactory\t \\\n> +{ \\\n> +public: \\\n> +\thelper##Factory() : CameraSensorHelperFactory(name) {}\t \\\n> +\t\t\t\t\t\t\t\t \\\n> +private: \\\n> +\tCameraSensorHelper *createInstance()\t\t\t \\\n> +\t{\t\t\t\t\t\t\t \\\n> +\t\treturn new helper();\t\t\t\t \\\n> +\t}\t\t\t\t\t\t\t \\\n> +};\t\t\t\t\t\t\t\t \\\n> +static helper##Factory global_##helper##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 038fc490..dc90542c 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,11 +2,13 @@\n> \n> libipa_headers = files([\n> 'algorithm.h',\n> + 'camera_sensor_helper.h',\n> 'histogram.h'\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'camera_sensor_helper.cpp',\n> 'histogram.cpp'\n> ])\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AAFA6C321F\n\tfor ;\n\tTue, 29 Jun 2021 12:44:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B2B2684EA;\n\tTue, 29 Jun 2021 14:44:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 270F3684CB\n\tfor ;\n\tTue, 29 Jun 2021 14:44:54 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD4E94AD;\n\tTue, 29 Jun 2021 14:44:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u6rtgI+n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624970693;\n\tbh=z3HoIEt2BdYaaoZDEeNkr9eDtPn/jba5uzsMfuJdg1E=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=u6rtgI+negWXANYV74Py2PrEBzlRTmoweca92EFTjhor+3THpmjO9obscfxO30YoS\n\tFzwCBctLISekx6/BTzzRB47LCmvvrJyahLmVp47CO8kVMxA0zQGTmhBldXGYf7umAo\n\tNpR3knHGdUlPXEnvXBiTZAzdzsxPVcA0wsoyyuJ8=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Tue, 29 Jun 2021 13:44:51 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17921,"web_url":"https://patchwork.libcamera.org/comment/17921/","msgid":"","date":"2021-06-29T15:40:06","subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 29/06/2021 14:44, Kieran Bingham wrote:\n> Hi JM,\n> \n> On 22/06/2021 10:45, Jean-Michel Hautbois wrote:\n>> For various sensor operations, it may be needed to do sensor specific\n>> computations, like analogue gain or vertical blanking.\n>>\n>> This commit introduces a new camera sensor helper in libipa which aims\n>> to solve this specific issue.\n>> It is based on the MIPI alliance Specification for Camera Command Set\n>> and implements, for now, only the analogue \"Global gain\" mode.\n>> Setting analogue gain for a specific sensor is not a straightforward\n>> operation, as one needs to know how the gain is calculated for it.\n>>\n>> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>>\n>> Adding a new sensor is pretty straightforward as one only needs to\n>> implement the sub-class for it and register that class to the\n>> CameraSensorHelperFactory.\n> \n> While extending the factories to add a new class seems simple, I'm\n> having a hard time to determine what the 'correct' way to extend it is.\n> \n> How would I determine the correct values for each of the fields for\n> instance?\n> \n> Is there any guidance on where this information comes from or how it\n> should be determined? or are they values you already had somewhere?\n> \n> For instance, we now need to add the ov13858, which is on the Soraka.\n> \n> What should we follow to add that ?\n\nYou need to determine the model used on your sensor (Linear or\nExponential, or something else :-)).\nIn case of imx219, the datasheet explicitly gives the m0/m1 and c0/c1\nvalues, so it is easy :-).\nIn OV5693, the datasheet mentions a *16 gain so the parameters are\ndefined to have gainCode = 16 * gain / 1.\n\n> \n> I've added defaults by copying another as an example, but that's no\n> different to us supplying incorrect defaults otherwise.\n> \n> I think outright failure when this data isn't provided is a better\n> option here though (which is what has happened to me), as it forces the\n> user to find out how to get the right values, and make sure they are\n> added - but the easier it is to demonstrate how to do that correctly,\n> the better.\n> \n> --\n> Kieran\n> \n> \n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> Reviewed-by: Jacopo Mondi \n>> ---\n>> src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++\n>> src/ipa/libipa/camera_sensor_helper.h | 87 +++++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 407 insertions(+)\n>> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n>> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>> new file mode 100644\n>> index 00000000..a114ee73\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -0,0 +1,318 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n>> + */\n>> +#include \"camera_sensor_helper.h\"\n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +/**\n>> + * \\file camera_sensor_helper.h\n>> + * \\brief Helper class that performs sensor-specific parameter computations\n>> + *\n>> + * Computation of sensor configuration parameters is a sensor specific\n>> + * operation. Each CameraHelper derived class computes the value of\n>> + * configuration parameters, for example the analogue gain value, using\n>> + * sensor-specific functions and constants.\n>> + *\n>> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n>> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\class CameraSensorHelper\n>> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n>> + *\n>> + * CameraSensorHelper derived class instances are sensor specific. Each\n>> + * supported sensor will have an associated base class defined.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a CameraSensorHelper instance\n>> + *\n>> + * CameraSensorHelper derived class instances shall never be constructed manually\n>> + * but always through the CameraSensorHelperFactory::create() method.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Compute gain code from the analogue gain absolute value\n>> + * \\param[in] gain The real gain to pass\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + *\n>> + * \\return The gain code to pass to V4l2\n>> + */\n>> +uint32_t CameraSensorHelper::gainCode(double gain) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n>> + * \\param[in] gainCode The V4l2 subdev control gain\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations. It is the counterpart of the function\n>> + * CameraSensorHelper::gainCode.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + *\n>> + * \\return The real gain\n>> + */\n>> +double CameraSensorHelper::gain(uint32_t gainCode) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n>> +\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n>> +}\n>> +\n>> +/**\n>> + * \\enum CameraSensorHelper::AnalogueGainType\n>> + * \\brief the gain calculation modes as defined by the MIPI CCS\n>> + *\n>> + * Describes the image sensor analogue gain capabilities.\n>> + * Two modes are possible, depending on the sensor: Global and Alternate.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainLinear\n>> + * \\brief Gain is computed using linear gain estimation\n>> + *\n>> + * The relationship between the integer gain parameter and the resulting gain\n>> + * multiplier is given by the following equation:\n>> + *\n>> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>> + *\n>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>> + * image-sensor-specific constants of the sensor.\n>> + * These constants are static parameters, and for any given image sensor either\n>> + * m0 or m1 shall be zero.\n>> + *\n>> + * The full Gain equation therefore reduces to either:\n>> + *\n>> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainExponential\n>> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n>> + *\n>> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n>> + * If the image sensor supports it, then the global analogue gain can be controlled\n>> + * by linear and exponential gain formula:\n>> + *\n>> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n>> + * \\todo not implemented in libipa\n>> + */\n>> +\n>> +/**\n>> + * \\struct CameraSensorHelper::AnalogueGainConstants\n>> + * \\brief Analogue gain constants used for gain calculation\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n>> + * \\brief Analogue gain calculation mode\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n>> + * \\brief Constant used in the analogue Gain coding/decoding\n>> + *\n>> + * \\note either m0 or m1 shall be zero.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + *\n>> + * \\note either m0 or m1 shall be zero.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants_\n>> + * \\brief The analogue gain parameters used for calculation\n>> + *\n>> + * The analogue gain is calculated through a formula, and its parameters are\n>> + * sensor specific. Use this variable to store the values at init time.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperFactory\n>> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n>> + *\n>> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n>> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n>> + * classes. Each CameraSensorHelper subclass shall register itself using the\n>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n>> + * instance of a CameraSensorHelperFactory subclass and register it with the\n>> + * static list of factories.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a camera sensor helper factory\n>> + * \\param[in] name Name of the camera sensor helper class\n>> + *\n>> + * Creating an instance of the factory registers it with the global list of\n>> + * factories, accessible through the factories() function.\n>> + *\n>> + * The factory \\a name is used for debug purpose and shall be unique.\n>> + */\n>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n>> +\t: name_(name)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + * \\param[in] name Name of the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n>> + * corresponding to the factory\n>> + */\n>> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tCameraSensorHelperFactory::factories();\n>> +\n>> +\tfor (CameraSensorHelperFactory *factory : factories) {\n>> +\t\tif (name != factory->name_)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tCameraSensorHelper *helper = factory->createInstance();\n>> +\t\treturn std::unique_ptr(helper);\n>> +\t}\n>> +\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Add a camera sensor helper class to the registry\n>> + * \\param[in] factory Factory to use to construct the camera sensor helper\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n>> + * name.\n>> + */\n>> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n>> +{\n>> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n>> +\n>> +\tfactories.push_back(factory);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all camera sensor helper factories\n>> + *\n>> + * The static factories map is defined inside the function to ensures it gets\n>> + * initialized on first use, without any dependency on link order.\n>> + *\n>> + * \\return The list of camera sensor helper factories\n>> + */\n>> +std::vector &CameraSensorHelperFactory::factories()\n>> +{\n>> +\tstatic std::vector factories;\n>> +\treturn factories;\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperFactory::createInstance()\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + *\n>> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n>> + * macro. It creates a camera sensor helper instance associated with the camera\n>> + * sensor model.\n>> + *\n>> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n>> + * subclass corresponding to the factory\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelperFactory::name_\n>> + * \\brief The name of the factory\n>> + */\n>> +\n>> +/**\n>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n>> + * \\param[in] name Sensor model name used to register the class\n>> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n>> + *\n>> + * Register a CameraSensorHelper subclass with the factory and make it available to\n>> + * try and match devices.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperImx219\n>> + * \\brief Create and give helpers for the imx219 sensor\n>> + */\n>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperImx219()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5670\n>> + * \\brief Create and give helpers for the ov5670 sensor\n>> + */\n>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5670()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5693\n>> + * \\brief Create and give helpers for the ov5693 sensor\n>> + */\n>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5693()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n>> new file mode 100644\n>> index 00000000..c43e4bf6\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>> @@ -0,0 +1,87 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelper() = default;\n>> +\tvirtual ~CameraSensorHelper() = default;\n>> +\n>> +\tvirtual uint32_t gainCode(double gain) const;\n>> +\tvirtual double gain(uint32_t gainCode) const;\n>> +\n>> +protected:\n>> +\tenum AnalogueGainType {\n>> +\t\tAnalogueGainLinear = 0,\n>> +\t\tAnalogueGainExponential = 2,\n>> +\t};\n>> +\n>> +\tstruct AnalogueGainConstants {\n>> +\t\tAnalogueGainType type;\n>> +\t\tint16_t m0;\n>> +\t\tint16_t c0;\n>> +\t\tint16_t m1;\n>> +\t\tint16_t c1;\n>> +\t};\n>> +\n>> +\tAnalogueGainConstants analogueGainConstants_;\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>> +};\n>> +\n>> +class CameraSensorHelperFactory\n>> +{\n>> +public:\n>> +\tCameraSensorHelperFactory(const std::string name);\n>> +\tvirtual ~CameraSensorHelperFactory() = default;\n>> +\n>> +\tstatic std::unique_ptr create(const std::string &name);\n>> +\n>> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n>> +\tstatic std::vector &factories();\n>> +\n>> +protected:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>> +\tvirtual CameraSensorHelper *createInstance() = 0;\n>> +\n>> +\tstd::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t \\\n>> +class helper##Factory final : public CameraSensorHelperFactory\t \\\n>> +{ \\\n>> +public: \\\n>> +\thelper##Factory() : CameraSensorHelperFactory(name) {}\t \\\n>> +\t\t\t\t\t\t\t\t \\\n>> +private: \\\n>> +\tCameraSensorHelper *createInstance()\t\t\t \\\n>> +\t{\t\t\t\t\t\t\t \\\n>> +\t\treturn new helper();\t\t\t\t \\\n>> +\t}\t\t\t\t\t\t\t \\\n>> +};\t\t\t\t\t\t\t\t \\\n>> +static helper##Factory global_##helper##Factory;\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 038fc490..dc90542c 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -2,11 +2,13 @@\n>> \n>> libipa_headers = files([\n>> 'algorithm.h',\n>> + 'camera_sensor_helper.h',\n>> 'histogram.h'\n>> ])\n>> \n>> libipa_sources = files([\n>> 'algorithm.cpp',\n>> + 'camera_sensor_helper.cpp',\n>> 'histogram.cpp'\n>> ])\n>> \n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B264CC321F\n\tfor ;\n\tTue, 29 Jun 2021 15:40:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20A60684E9;\n\tTue, 29 Jun 2021 17:40:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CD5C684CB\n\tfor ;\n\tTue, 29 Jun 2021 17:40:09 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:d4ff:199e:6bde:2ea6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B9134AD;\n\tTue, 29 Jun 2021 17:40:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JBs8Jcdg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624981208;\n\tbh=+dER26OTIHT/hf/ykcI7QLBzlwFpvj81tSpOC51lL0c=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=JBs8JcdgAHtRByo8p+WjyS72/saxpnUjkQhSUPxDFmse8tvB0brIoAXSwo5QVrVHd\n\tprx/x71X1sIPuSQBnTgHaCzxG5WS9Uzrt7KWY1YW30BcFATwDNaFgpTA63qFNVrBRY\n\tuVoUhevVWo2BlkN7qpkPPqazkPPBM0a0XkEAvtlU=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210622094523.42915-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210622094523.42915-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Tue, 29 Jun 2021 17:40:06 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]