[{"id":21606,"web_url":"https://patchwork.libcamera.org/comment/21606/","msgid":"","date":"2021-12-06T11:46:11","subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Dec 06, 2021 at 02:49:17PM +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\nAs the type is fairly long, we could shorten lines by renaming the\nparameter from testPatternMode to mode (here and below, but not the\ntestPatternMode_ member). Up to you.\n\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\nI think I recall someone proposing renaming this function to\napplyTestPatternMode(), and I think that's a good idea, as that's what\nit does.\n\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\nI'd drop this, staticProps_ is set by the caller.\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\nIt could also be that none of the test patterns supported by the sensor\nmatch the test patterns standardized by libcamera. We'll eventually want\nto add support for extra patterns (and possibly for custom patterns),\nbut until we do so, I'd turn this into a Debug message.\n\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\nLet's keep the brief brief :-)\n\n * \\brief Set the test pattern mode for the camera sensor\n * \\param[in] testPatternMode The test pattern mode\n *\n * The new \\a testPatternMode mode is applied to the sensor if it differs from\n * the active test pattern mode. Otherwise, this function is a no-op. Setting\n * the same test pattern mode for every frame thus incurs no performance\n * penalty.\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> + * \\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\nI wonder if we should turn testPatternModes_ into a\nstd::map. The lookup will\nbe more efficient, and we'll be able to drop the lookup in staticProps_\nbelow. This patch could thus be simplified. This could be done on top\nthough.\n\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\ns/mode:/mode/\n\nReviewed-by: Laurent Pinchart \n\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","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 0E982BDB13\n\tfor ;\n\tMon, 6 Dec 2021 11:46:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 660876086C;\n\tMon, 6 Dec 2021 12:46:40 +0100 (CET)","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 CFB5C60725\n\tfor ;\n\tMon, 6 Dec 2021 12:46:38 +0100 (CET)","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 4EA1DEE;\n\tMon, 6 Dec 2021 12:46:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GSvSAXzI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638791198;\n\tbh=VqoQ6F1Ngf3/3PS6VRG01by8noFwMPMmntHLkpRV4MA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GSvSAXzIGQxhfO6kRBgv4dEoEv5PWyXN8tShXZP7PxjoLtgvlrEx9RAluuRuz7NTa\n\ttt5yAywoCbBGSIgCmMBBCeUi4MbbCTeMt5/4qqbiTW6X0wr5G5xgF+02I8DOvhBltq\n\tLqsrsig+772nJcJEGptr/h68Iw/bPIpVgWuOj/FA=","Date":"Mon, 6 Dec 2021 13:46:11 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211206054918.2467049-1-hiroh@chromium.org>\n\t<20211206054918.2467049-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211206054918.2467049-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v10 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\" "}}]