[{"id":21271,"web_url":"https://patchwork.libcamera.org/comment/21271/","msgid":"<20211126084431.6ewiktscefwha77v@uno.localdomain>","date":"2021-11-26T08:44:31","subject":"Re: [libcamera-devel] [PATCH v8 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 Fri, Nov 26, 2021 at 02:08:09PM +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 | 6 ++\n> src/libcamera/camera_sensor.cpp | 75 ++++++++++++++++++++--\n> 2 files changed, 74 insertions(+), 7 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index edef2220..a110fd08 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> @@ -82,6 +85,8 @@ private:\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 +94,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..2c6312d4 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,6 +161,21 @@ int CameraSensor::init()\n> \tif (ret)\n> \t\treturn ret;\n>\n> +\tauto initTestPatternMode = controls::draft::TestPatternModeEnum::TestPatternModeOff;\n> +\tif (staticProps_) {\n> +\t\tauto it = staticProps_->testPatternModes.find(initTestPatternMode);\n> +\t\tif (it != staticProps_->testPatternModes.end()) {\n\nCan a sensor not support ModeOff ?\n\n> +\t\t\tControlList ctrls{ controls() };\n> +\t\t\tctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> +\n> +\t\t\tret = setControls(&ctrls);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\ttestPatternMode_ = initTestPatternMode;\n> +\t\t}\n> +\t}\n\nCan't you do so in initTestPatternMode() instead of pooluting init() ?\nYou can skip a lot of checks if you move the code in that function.\n\n> +\n> \treturn 0;\n> }\n>\n> @@ -300,14 +315,14 @@ 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(staticProps_->testPatternModes);\n\nAlready there, but what's the point of passing a memeber of\nstaticProps_ to a function of the same class ? initTestPatternMode()\nhas access to staticProps_ and now that it has been made a class\nmember you can drop the paramter\n\n> }\n>\n> void CameraSensor::initTestPatternModes(\n> @@ -315,7 +330,17 @@ 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> +\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 +556,42 @@ 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\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\ntestPatternMode_ is not initialized if !staticProps_ or\ntestPatternMode_.empty(). I would move this check after the below one\n\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> +\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 47D0FBDB13\n\tfor ;\n\tFri, 26 Nov 2021 08:43:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79274604FA;\n\tFri, 26 Nov 2021 09:43:42 +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 BE4E160227\n\tfor ;\n\tFri, 26 Nov 2021 09:43:40 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 133D9240002;\n\tFri, 26 Nov 2021 08:43:39 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 09:44:31 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211126084431.6ewiktscefwha77v@uno.localdomain>","References":"<20211126050810.1871781-1-hiroh@chromium.org>\n\t<20211126050810.1871781-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211126050810.1871781-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v8 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\" "}}]