[{"id":21529,"web_url":"https://patchwork.libcamera.org/comment/21529/","msgid":"<20211201144542.jxh4jek62422apez@uno.localdomain>","date":"2021-12-01T14:45:42","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Nov 29, 2021 at 05:34:23PM +0900, Hirokazu Honda wrote:\n> This adds a function to set a camera sensor driver a test pattern\n> mode. CameraSensor initializes the test pattern mode by Off.\n>\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Kieran Bingham \n> ---\n> include/libcamera/internal/camera_sensor.h | 11 ++-\n> src/libcamera/camera_sensor.cpp | 82 +++++++++++++++++++---\n> 2 files changed, 80 insertions(+), 13 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 310571ca..3362eaff 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -28,6 +28,8 @@ namespace libcamera {\n> class BayerFormat;\n> class MediaEntity;\n>\n> +struct CameraSensorProperties;\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:\n> @@ -47,6 +49,7 @@ public:\n> \t{\n> \t\treturn testPatternModes_;\n> \t}\n> +\tint setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n>\n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -73,15 +76,16 @@ private:\n> \tint validateSensorDriver();\n> \tvoid initVimcDefaultProperties();\n> \tvoid initStaticProperties();\n> -\tvoid initTestPatternModes(\n> -\t\tconst std::map\n> -\t\t\t&testPatternModeMap);\n> +\tvoid initTestPatternModes();\n> \tint initProperties();\n> +\tint initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n>\n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> \tunsigned int pad_;\n>\n> +\tconst CameraSensorProperties *staticProps_;\n> +\n> \tstd::string model_;\n> \tstd::string id_;\n>\n> @@ -89,6 +93,7 @@ private:\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> \tstd::vector testPatternModes_;\n> +\tcontrols::draft::TestPatternModeEnum testPatternMode_;\n\nThis doesn't seem to be initialized. I understand this will be set by\nthe first call to initTestPatternMode() which happens at init() time,\nbut I wonder if Coverity or other analysis tools might complain in\nthis case.\n\n>\n> \tSize pixelArraySize_;\n> \tRectangle activeArea_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f0aa9f24..8a79b970 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -54,8 +54,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> * Once constructed the instance must be initialized with init().\n> */\n> CameraSensor::CameraSensor(const MediaEntity *entity)\n> -\t: entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> -\t properties_(properties::properties)\n> +\t: entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> +\t bayerFormat_(nullptr), properties_(properties::properties)\n> {\n> }\n>\n> @@ -161,7 +161,7 @@ int CameraSensor::init()\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\treturn 0;\n> +\treturn initTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> }\n>\n> int CameraSensor::validateSensorDriver()\n> @@ -300,22 +300,33 @@ void CameraSensor::initVimcDefaultProperties()\n>\n> void CameraSensor::initStaticProperties()\n> {\n> -\tconst CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> -\tif (!props)\n> +\tstaticProps_ = CameraSensorProperties::get(model_);\n> +\tif (!staticProps_)\n> \t\treturn;\n>\n> \t/* Register the properties retrieved from the sensor database. */\n> -\tproperties_.set(properties::UnitCellSize, props->unitCellSize);\n> +\tproperties_.set(properties::UnitCellSize, staticProps_->unitCellSize);\n>\n> -\tinitTestPatternModes(props->testPatternModes);\n> +\tinitTestPatternModes();\n> }\n>\n> -void CameraSensor::initTestPatternModes(\n> -\tconst std::map &testPatternModes)\n> +void CameraSensor::initTestPatternModes()\n> {\n> \tconst auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> \tif (v4l2TestPattern == controls().end()) {\n> -\t\tLOG(CameraSensor, Debug) << \"No static test pattern map for \\'\"\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"V4L2_CID_TEST_PATTERN is not supported\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tASSERT(staticProps_);\n\nWe can't get here it !staticProps_\n\n> +\tconst auto &testPatternModes = staticProps_->testPatternModes;\n> +\tif (testPatternModes.empty()) {\n> +\t\t/*\n> +\t\t * The camera sensor supports test patterns but we don't know\n> +\t\t * how to map them so this should be fixed.\n> +\t\t */\n> +\t\tLOG(CameraSensor, Error) << \"No static test pattern map for \\'\"\n> \t\t\t\t\t << model() << \"\\'\";\n> \t\treturn;\n> \t}\n> @@ -531,6 +542,57 @@ Size CameraSensor::resolution() const\n> * \\return The list of test pattern modes\n> */\n>\n> +/**\n> + * \\brief Set the test pattern mode for the camera sensor if it is not the\n> + * currently set test pattern mode.\n> + * \\param[in] testPatternMode Test pattern mode control value to set the camera\n> + * sensor\n\nDrop blank lines between param and return if there is no additional\ndescription\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)\n> +{\n> +\tif (testPatternMode_ == testPatternMode)\n> +\t\treturn 0;\n> +\n> +\treturn initTestPatternMode(testPatternMode);\n> +}\n> +\n> +/**\n> + * \\brief Set the test pattern mode for the camera sensor\n> + * \\param[in] testPatternMode Test pattern mode control value to set the camera\n> + * sensor\n> + *\n\nDitto\n\n> + * \\return 0 on success or a negative error code otherwise\n * \\retval -EINVAL \\a testPatternMode is not supported by the sensor\n> + */\n> +int CameraSensor::initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)\n> +{\n> +\tif (!staticProps_ || testPatternModes_.empty())\n> +\t\treturn 0;\n\nChecking if testPatternModes_ is empty should be enough, but that's\nnot a big deal\n\n> +\n> +\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> +\t\t\t testPatternMode);\n> +\tif (it == testPatternModes_.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> +\t\t\t\t\t << testPatternMode;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tLOG(CameraSensor, Debug) << \"Apply test pattern mode: \" << testPatternMode;\n> +\n> +\tint32_t index = staticProps_->testPatternModes.at(testPatternMode);\n> +\tControlList ctrls{ controls() };\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> +\n> +\tint ret = setControls(&ctrls);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttestPatternMode_ = testPatternMode;\n> +\n> +\treturn 0;\n> +}\n> +\n\nAll minors that could be fixed while applying\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> --\n> 2.34.0.rc2.393.gf8c9666880-goog\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 838FBBF415\n\tfor ;\n\tWed, 1 Dec 2021 14:44:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0239160732;\n\tWed, 1 Dec 2021 15:44:53 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99571604FC\n\tfor ;\n\tWed, 1 Dec 2021 15:44:51 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id AB8E82000D;\n\tWed, 1 Dec 2021 14:44:50 +0000 (UTC)"],"Date":"Wed, 1 Dec 2021 15:45:42 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211201144542.jxh4jek62422apez@uno.localdomain>","References":"<20211129083424.3136533-1-hiroh@chromium.org>\n\t<20211129083424.3136533-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129083424.3136533-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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":21530,"web_url":"https://patchwork.libcamera.org/comment/21530/","msgid":"<20211201144755.xglgqxrfvouz4gff@uno.localdomain>","date":"2021-12-01T14:47:55","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Nov 29, 2021 at 05:34:23PM +0900, Hirokazu Honda wrote:\n> This adds a function to set a camera sensor driver a test pattern\n> mode. CameraSensor initializes the test pattern mode by Off.\n>\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Kieran Bingham \n> ---\n> include/libcamera/internal/camera_sensor.h | 11 ++-\n> src/libcamera/camera_sensor.cpp | 82 +++++++++++++++++++---\n> 2 files changed, 80 insertions(+), 13 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 310571ca..3362eaff 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -28,6 +28,8 @@ namespace libcamera {\n> class BayerFormat;\n> class MediaEntity;\n>\n> +struct CameraSensorProperties;\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:\n> @@ -47,6 +49,7 @@ public:\n> \t{\n> \t\treturn testPatternModes_;\n> \t}\n> +\tint setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n>\n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -73,15 +76,16 @@ private:\n> \tint validateSensorDriver();\n> \tvoid initVimcDefaultProperties();\n> \tvoid initStaticProperties();\n> -\tvoid initTestPatternModes(\n> -\t\tconst std::map\n> -\t\t\t&testPatternModeMap);\n> +\tvoid initTestPatternModes();\n\nAlso I think this should be called applyTestPatternMode() as it is\ncalled by setTestPatternMode()\n\n> \tint initProperties();\n> +\tint initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n>\n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> \tunsigned int pad_;\n>\n> +\tconst CameraSensorProperties *staticProps_;\n> +\n> \tstd::string model_;\n> \tstd::string id_;\n>\n> @@ -89,6 +93,7 @@ private:\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> \tstd::vector testPatternModes_;\n> +\tcontrols::draft::TestPatternModeEnum testPatternMode_;\n>\n> \tSize pixelArraySize_;\n> \tRectangle activeArea_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f0aa9f24..8a79b970 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -54,8 +54,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> * Once constructed the instance must be initialized with init().\n> */\n> CameraSensor::CameraSensor(const MediaEntity *entity)\n> -\t: entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> -\t properties_(properties::properties)\n> +\t: entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> +\t bayerFormat_(nullptr), properties_(properties::properties)\n> {\n> }\n>\n> @@ -161,7 +161,7 @@ int CameraSensor::init()\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\treturn 0;\n> +\treturn initTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> }\n>\n> int CameraSensor::validateSensorDriver()\n> @@ -300,22 +300,33 @@ void CameraSensor::initVimcDefaultProperties()\n>\n> void CameraSensor::initStaticProperties()\n> {\n> -\tconst CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> -\tif (!props)\n> +\tstaticProps_ = CameraSensorProperties::get(model_);\n> +\tif (!staticProps_)\n> \t\treturn;\n>\n> \t/* Register the properties retrieved from the sensor database. */\n> -\tproperties_.set(properties::UnitCellSize, props->unitCellSize);\n> +\tproperties_.set(properties::UnitCellSize, staticProps_->unitCellSize);\n>\n> -\tinitTestPatternModes(props->testPatternModes);\n> +\tinitTestPatternModes();\n> }\n>\n> -void CameraSensor::initTestPatternModes(\n> -\tconst std::map &testPatternModes)\n> +void CameraSensor::initTestPatternModes()\n> {\n> \tconst auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> \tif (v4l2TestPattern == controls().end()) {\n> -\t\tLOG(CameraSensor, Debug) << \"No static test pattern map for \\'\"\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"V4L2_CID_TEST_PATTERN is not supported\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tASSERT(staticProps_);\n> +\tconst auto &testPatternModes = staticProps_->testPatternModes;\n> +\tif (testPatternModes.empty()) {\n> +\t\t/*\n> +\t\t * The camera sensor supports test patterns but we don't know\n> +\t\t * how to map them so this should be fixed.\n> +\t\t */\n> +\t\tLOG(CameraSensor, Error) << \"No static test pattern map for \\'\"\n> \t\t\t\t\t << model() << \"\\'\";\n> \t\treturn;\n> \t}\n> @@ -531,6 +542,57 @@ Size CameraSensor::resolution() const\n> * \\return The list of test pattern modes\n> */\n>\n> +/**\n> + * \\brief Set the test pattern mode for the camera sensor if it is not the\n> + * currently set test pattern mode.\n> + * \\param[in] testPatternMode Test pattern mode control value to set the camera\n> + * sensor\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)\n> +{\n> +\tif (testPatternMode_ == testPatternMode)\n> +\t\treturn 0;\n> +\n> +\treturn initTestPatternMode(testPatternMode);\n> +}\n> +\n> +/**\n> + * \\brief Set the test pattern mode for the camera sensor\n> + * \\param[in] testPatternMode Test pattern mode control value to set the camera\n> + * sensor\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)\n> +{\n> +\tif (!staticProps_ || testPatternModes_.empty())\n> +\t\treturn 0;\n> +\n> +\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> +\t\t\t testPatternMode);\n> +\tif (it == testPatternModes_.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> +\t\t\t\t\t << testPatternMode;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tLOG(CameraSensor, Debug) << \"Apply test pattern mode: \" << testPatternMode;\n> +\n> +\tint32_t index = staticProps_->testPatternModes.at(testPatternMode);\n> +\tControlList ctrls{ controls() };\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> +\n> +\tint ret = setControls(&ctrls);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttestPatternMode_ = testPatternMode;\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> --\n> 2.34.0.rc2.393.gf8c9666880-goog\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 EBFCDBF415\n\tfor ;\n\tWed, 1 Dec 2021 14:47:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 622B86073C;\n\tWed, 1 Dec 2021 15:47:05 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AAB5604FC\n\tfor ;\n\tWed, 1 Dec 2021 15:47:04 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 8ADF4240015;\n\tWed, 1 Dec 2021 14:47:03 +0000 (UTC)"],"Date":"Wed, 1 Dec 2021 15:47:55 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211201144755.xglgqxrfvouz4gff@uno.localdomain>","References":"<20211129083424.3136533-1-hiroh@chromium.org>\n\t<20211129083424.3136533-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129083424.3136533-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","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\" "}}]