[{"id":21171,"web_url":"https://patchwork.libcamera.org/comment/21171/","msgid":"","date":"2021-11-24T04:53:17","subject":"Re: [libcamera-devel] [PATCH v7 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 Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:\n> This adds a function to set a camera sensor driver a test pattern\n> mode.\n> \n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n\nThe patch I've reviewed differs significantly from this one. Please drop\nmy tag in v8.\n\n> ---\n> include/libcamera/internal/camera_sensor.h | 7 +++\n> src/libcamera/camera_sensor.cpp | 60 +++++++++++++++++++---\n> src/libcamera/control_ids.yaml | 5 ++\n> 3 files changed, 65 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index edef2220..f355f323 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -27,6 +27,9 @@ namespace libcamera {\n> \n> class BayerFormat;\n> class MediaEntity;\n> +class Request;\n\nNot needed anymore.\n\n> +\n> +struct CameraSensorProperties;\n> \n> class CameraSensor : protected Loggable\n> {\n> @@ -47,6 +50,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 +86,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 +95,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..e1293980 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \n> +#include \n\nNot needed anymore either.\n\n> \n> #include \n> \n> @@ -54,8 +55,9 @@ 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 testPatternMode_(controls::draft::TestPatternModeUnset),\n> +\t bayerFormat_(nullptr), properties_(properties::properties)\n> {\n> }\n> \n> @@ -300,14 +302,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> }\n> \n> void CameraSensor::initTestPatternModes(\n> @@ -315,7 +317,15 @@ 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// The camera sensor supports test patterns but we don't know\n> +\t\t// how to map them so this should be fixed.\n\nWe use C-style comments.\n\n> +\t\tLOG(CameraSensor, Error) << \"No static test pattern map for \\'\"\n\nNo need to escape the single quote.\n\n> \t\t\t\t\t << model() << \"\\'\";\n> \t\treturn;\n> \t}\n> @@ -531,6 +541,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> +\n> +\tif (!staticProps_ || testPatternModes_.empty())\n> +\t\treturn 0;\n\nShouldn't this return an error ?\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> +\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\nThis brings the question of where control values should be cached, to\navoid applying controls that haven't changed. I don't think this need is\nlimited to the CameraSensor class, so a more generic solution at the\npipeline handler level may be better. This doesn't have to be addressed\nnow though.\n\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> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 9d4638ae..083bac7b 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -639,6 +639,11 @@ controls:\n> Control to select the test pattern mode. Currently identical to\n> ANDROID_SENSOR_TEST_PATTERN_MODE.\n> enum:\n> + - name: TestPatternModeUnset\n> + value: -1\n> + description: |\n> + No test pattern is set. Returned frames by the camera device are\n> + undefined.\n\nWhy do we need this ? It's not documented in the commit message. If we\nneed this new value, its addition should be split to a separate commit,\nwith a proper explanation.\n\n> - name: TestPatternModeOff\n> value: 0\n> description: |","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 4A31EBDB13\n\tfor ;\n\tWed, 24 Nov 2021 04:53:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F6B76022F;\n\tWed, 24 Nov 2021 05:53:41 +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 BE8E560121\n\tfor ;\n\tWed, 24 Nov 2021 05:53:40 +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 23179D78;\n\tWed, 24 Nov 2021 05:53:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Zh8dwjBb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637729620;\n\tbh=8lq/FCo93BUZKzj61iAXArt1p9MKQuTzemNg5b3n9RU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zh8dwjBbE43B2DecXJN2VufqcZbn6dv122G1DNZVTZzs0dwTyxi0hkmOnzYxUPZw/\n\tz5Fo3sJt2RMOuGSkoQrRJRNyfR3JDd8nMAK8mXrB03mVYb7yd7cLa9FeRjZoNgI9ip\n\tgJXIEMqdH/xq8lpO3OuCd0Dt+LCpzbhE/PUoRIQc=","Date":"Wed, 24 Nov 2021 06:53:17 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211123190812.69805-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v7 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":21173,"web_url":"https://patchwork.libcamera.org/comment/21173/","msgid":"","date":"2021-11-24T05:10:11","subject":"Re: [libcamera-devel] [PATCH v7 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:\n> > This adds a function to set a camera sensor driver a test pattern\n> > mode.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Laurent Pinchart \n>\n> The patch I've reviewed differs significantly from this one. Please drop\n> my tag in v8.\n>\n> > ---\n> > include/libcamera/internal/camera_sensor.h | 7 +++\n> > src/libcamera/camera_sensor.cpp | 60 +++++++++++++++++++---\n> > src/libcamera/control_ids.yaml | 5 ++\n> > 3 files changed, 65 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index edef2220..f355f323 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -27,6 +27,9 @@ namespace libcamera {\n> >\n> > class BayerFormat;\n> > class MediaEntity;\n> > +class Request;\n>\n> Not needed anymore.\n>\n> > +\n> > +struct CameraSensorProperties;\n> >\n> > class CameraSensor : protected Loggable\n> > {\n> > @@ -47,6 +50,7 @@ public:\n> > {\n> > return testPatternModes_;\n> > }\n> > + int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n> >\n> > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > const Size &size) const;\n> > @@ -82,6 +86,8 @@ private:\n> > std::unique_ptr subdev_;\n> > unsigned int pad_;\n> >\n> > + const CameraSensorProperties *staticProps_;\n> > +\n> > std::string model_;\n> > std::string id_;\n> >\n> > @@ -89,6 +95,7 @@ private:\n> > std::vector mbusCodes_;\n> > std::vector sizes_;\n> > std::vector testPatternModes_;\n> > + controls::draft::TestPatternModeEnum testPatternMode_;\n> >\n> > Size pixelArraySize_;\n> > Rectangle activeArea_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index f0aa9f24..e1293980 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -17,6 +17,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n>\n> Not needed anymore either.\n>\n> >\n> > #include \n> >\n> > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> > * Once constructed the instance must be initialized with init().\n> > */\n> > CameraSensor::CameraSensor(const MediaEntity *entity)\n> > - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> > - properties_(properties::properties)\n> > + : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> > + testPatternMode_(controls::draft::TestPatternModeUnset),\n> > + bayerFormat_(nullptr), properties_(properties::properties)\n> > {\n> > }\n> >\n> > @@ -300,14 +302,14 @@ void CameraSensor::initVimcDefaultProperties()\n> >\n> > void CameraSensor::initStaticProperties()\n> > {\n> > - const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > - if (!props)\n> > + staticProps_ = CameraSensorProperties::get(model_);\n> > + if (!staticProps_)\n> > return;\n> >\n> > /* Register the properties retrieved from the sensor database. */\n> > - properties_.set(properties::UnitCellSize, props->unitCellSize);\n> > + properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);\n> >\n> > - initTestPatternModes(props->testPatternModes);\n> > + initTestPatternModes(staticProps_->testPatternModes);\n> > }\n> >\n> > void CameraSensor::initTestPatternModes(\n> > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(\n> > {\n> > const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> > if (v4l2TestPattern == controls().end()) {\n> > - LOG(CameraSensor, Debug) << \"No static test pattern map for \\'\"\n> > + LOG(CameraSensor, Debug)\n> > + << \"V4L2_CID_TEST_PATTERN is not supported\";\n> > + return;\n> > + }\n> > +\n> > + if (testPatternModes.empty()) {\n> > + // The camera sensor supports test patterns but we don't know\n> > + // how to map them so this should be fixed.\n>\n> We use C-style comments.\n>\n> > + LOG(CameraSensor, Error) << \"No static test pattern map for \\'\"\n>\n> No need to escape the single quote.\n>\n> > << model() << \"\\'\";\n> > return;\n> > }\n> > @@ -531,6 +541,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> > + if (testPatternMode_ == testPatternMode)\n> > + return 0;\n> > +\n> > + if (!staticProps_ || testPatternModes_.empty())\n> > + return 0;\n>\n> Shouldn't this return an error ?\n>\n> > +\n> > + auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > + testPatternMode);\n> > + if (it == testPatternModes_.end()) {\n> > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> > + << testPatternMode;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + int32_t index = staticProps_->testPatternModes.at(testPatternMode);\n> > + ControlList ctrls{ controls() };\n> > + ctrls.set(V4L2_CID_TEST_PATTERN, index);\n> > +\n> > + int ret = setControls(&ctrls);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + testPatternMode_ = testPatternMode;\n>\n> This brings the question of where control values should be cached, to\n> avoid applying controls that haven't changed. I don't think this need is\n> limited to the CameraSensor class, so a more generic solution at the\n> pipeline handler level may be better. This doesn't have to be addressed\n> now though.\n>\n> > +\n> > + return 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> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9d4638ae..083bac7b 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -639,6 +639,11 @@ controls:\n> > Control to select the test pattern mode. Currently identical to\n> > ANDROID_SENSOR_TEST_PATTERN_MODE.\n> > enum:\n> > + - name: TestPatternModeUnset\n> > + value: -1\n> > + description: |\n> > + No test pattern is set. Returned frames by the camera device are\n> > + undefined.\n>\n> Why do we need this ? It's not documented in the commit message. If we\n> need this new value, its addition should be split to a separate commit,\n> with a proper explanation.\n>\n\nIt was suggested by Kieran.\nI originally used -1 for the initialized value of a test pattern mode\nin CameraSensor.\n-1 lets the first setTestPatternMode(Off) be executed.\nIt seems hack as -1 is invalid test pattern mode. So Kieran would like\nto introduce this value.\n\nIf we move the cache of testpatternmode to ipu3 pipeline handler, this\nmay is not necessary.\n\nRegards,\n-Hiro\n> > - name: TestPatternModeOff\n> > value: 0\n> > description: |\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 C84F5BDB13\n\tfor ;\n\tWed, 24 Nov 2021 05:10:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C2EA6033C;\n\tWed, 24 Nov 2021 06:10:24 +0100 (CET)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E531B60121\n\tfor ;\n\tWed, 24 Nov 2021 06:10:22 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id x15so4848943edv.1\n\tfor ;\n\tTue, 23 Nov 2021 21:10:22 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"DLy1gnFk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Av7pziX+9UA+zmWTARGICoKvIa+M08ncAUlwHL0MMaE=;\n\tb=DLy1gnFkt4ZKH6VNu3xKJrFGHce7Sv1UkO+a+RdNxgNqD4XIegpQug6iDBiee/zsqf\n\tTGfAmg04blZ5Wu79aLNf/Fle3N1LYDQoC+G/rtsmpL0k5irzDEaEWQP6BtrxIRZW3MYz\n\tiZhx86SZsUnUBtzY3C/HbDE1R6flkBLCaOUPI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Av7pziX+9UA+zmWTARGICoKvIa+M08ncAUlwHL0MMaE=;\n\tb=vrHFgGdzaEAiNlCThOcTD863tMUZU3k1E2ehrx8e0NYqLtRurbSiui82aGDBpBYFj5\n\t32td64Fmf6OxjpK15GcYSgqpykM5Y0XrryEQDxhQPyUszM923X7Oeq2V0pyCSZKs2NYF\n\t8ZrmyZDOs+lVdB+IN1p5PkbQz4CEH+PgTR0ub7aESpDy+EktieznLNgOwRh0ps33uaPa\n\tJ6OVDEdjn85QqbkPO66b9h+AYWrh+bBYI74RSU/UUPiea6qeD+Uq/tqevLZB+UMYtw7n\n\tlO52AIbiQzSmltXdPJtnCKxLHnoh04rrCMDQ+3IyUTOubJbMRVWjPCgxj+FDLS30L9uU\n\tUDrA==","X-Gm-Message-State":"AOAM5338AGImi536i1zW0ZO9dPvnG9gzyp6hBdGKj+tsXZRTo/PqCbh4\n\t1lhI29XIwwlA1+6C7gslF6Uh7u1tHovNlE3rK96fVA==","X-Google-Smtp-Source":"ABdhPJyzmeaXTDT3Jelnmc2h9K87Ve/Hx+wHmip7YopSrvjqrKt3UmAj3ZTEZWfDZjik7a4LvKWBc47S+YpnbCePojc=","X-Received":"by 2002:a17:906:1456:: with SMTP id\n\tq22mr16010765ejc.291.1637730622464; \n\tTue, 23 Nov 2021 21:10:22 -0800 (PST)","MIME-Version":"1.0","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 24 Nov 2021 14:10:11 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v7 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":21191,"web_url":"https://patchwork.libcamera.org/comment/21191/","msgid":"<163775298509.2984710.14907940678127203061@Monstersaurus>","date":"2021-11-24T11:23:05","subject":"Re: [libcamera-devel] [PATCH v7 2/3] libcamera: camera_sensor:\n\tEnable to set a test pattern mode","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hirokazu Honda (2021-11-24 05:10:11)\n> Hi Laurent,\n> \n> On Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:\n> > > This adds a function to set a camera sensor driver a test pattern\n> > > mode.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Kieran Bingham \n> > > Reviewed-by: Laurent Pinchart \n> >\n> > The patch I've reviewed differs significantly from this one. Please drop\n> > my tag in v8.\n> >\n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 7 +++\n> > > src/libcamera/camera_sensor.cpp | 60 +++++++++++++++++++---\n> > > src/libcamera/control_ids.yaml | 5 ++\n> > > 3 files changed, 65 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index edef2220..f355f323 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -27,6 +27,9 @@ namespace libcamera {\n> > >\n> > > class BayerFormat;\n> > > class MediaEntity;\n> > > +class Request;\n> >\n> > Not needed anymore.\n> >\n> > > +\n> > > +struct CameraSensorProperties;\n> > >\n> > > class CameraSensor : protected Loggable\n> > > {\n> > > @@ -47,6 +50,7 @@ public:\n> > > {\n> > > return testPatternModes_;\n> > > }\n> > > + int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n> > >\n> > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > const Size &size) const;\n> > > @@ -82,6 +86,8 @@ private:\n> > > std::unique_ptr subdev_;\n> > > unsigned int pad_;\n> > >\n> > > + const CameraSensorProperties *staticProps_;\n> > > +\n> > > std::string model_;\n> > > std::string id_;\n> > >\n> > > @@ -89,6 +95,7 @@ private:\n> > > std::vector mbusCodes_;\n> > > std::vector sizes_;\n> > > std::vector testPatternModes_;\n> > > + controls::draft::TestPatternModeEnum testPatternMode_;\n> > >\n> > > Size pixelArraySize_;\n> > > Rectangle activeArea_;\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index f0aa9f24..e1293980 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> >\n> > Not needed anymore either.\n> >\n> > >\n> > > #include \n> > >\n> > > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> > > * Once constructed the instance must be initialized with init().\n> > > */\n> > > CameraSensor::CameraSensor(const MediaEntity *entity)\n> > > - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> > > - properties_(properties::properties)\n> > > + : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> > > + testPatternMode_(controls::draft::TestPatternModeUnset),\n> > > + bayerFormat_(nullptr), properties_(properties::properties)\n> > > {\n> > > }\n> > >\n> > > @@ -300,14 +302,14 @@ void CameraSensor::initVimcDefaultProperties()\n> > >\n> > > void CameraSensor::initStaticProperties()\n> > > {\n> > > - const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > - if (!props)\n> > > + staticProps_ = CameraSensorProperties::get(model_);\n> > > + if (!staticProps_)\n> > > return;\n> > >\n> > > /* Register the properties retrieved from the sensor database. */\n> > > - properties_.set(properties::UnitCellSize, props->unitCellSize);\n> > > + properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);\n> > >\n> > > - initTestPatternModes(props->testPatternModes);\n> > > + initTestPatternModes(staticProps_->testPatternModes);\n> > > }\n> > >\n> > > void CameraSensor::initTestPatternModes(\n> > > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(\n> > > {\n> > > const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> > > if (v4l2TestPattern == controls().end()) {\n> > > - LOG(CameraSensor, Debug) << \"No static test pattern map for \\'\"\n> > > + LOG(CameraSensor, Debug)\n> > > + << \"V4L2_CID_TEST_PATTERN is not supported\";\n> > > + return;\n> > > + }\n> > > +\n> > > + if (testPatternModes.empty()) {\n> > > + // The camera sensor supports test patterns but we don't know\n> > > + // how to map them so this should be fixed.\n> >\n> > We use C-style comments.\n> >\n> > > + LOG(CameraSensor, Error) << \"No static test pattern map for \\'\"\n> >\n> > No need to escape the single quote.\n> >\n> > > << model() << \"\\'\";\n> > > return;\n> > > }\n> > > @@ -531,6 +541,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> > > + if (testPatternMode_ == testPatternMode)\n> > > + return 0;\n> > > +\n> > > + if (!staticProps_ || testPatternModes_.empty())\n> > > + return 0;\n> >\n> > Shouldn't this return an error ?\n> >\n> > > +\n> > > + auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > + testPatternMode);\n> > > + if (it == testPatternModes_.end()) {\n> > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> > > + << testPatternMode;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + int32_t index = staticProps_->testPatternModes.at(testPatternMode);\n> > > + ControlList ctrls{ controls() };\n> > > + ctrls.set(V4L2_CID_TEST_PATTERN, index);\n> > > +\n> > > + int ret = setControls(&ctrls);\n> > > + if (ret)\n> > > + return ret;\n> > > +\n> > > + testPatternMode_ = testPatternMode;\n> >\n> > This brings the question of where control values should be cached, to\n> > avoid applying controls that haven't changed. I don't think this need is\n> > limited to the CameraSensor class, so a more generic solution at the\n> > pipeline handler level may be better. This doesn't have to be addressed\n> > now though.\n> >\n> > > +\n> > > + return 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> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 9d4638ae..083bac7b 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -639,6 +639,11 @@ controls:\n> > > Control to select the test pattern mode. Currently identical to\n> > > ANDROID_SENSOR_TEST_PATTERN_MODE.\n> > > enum:\n> > > + - name: TestPatternModeUnset\n> > > + value: -1\n> > > + description: |\n> > > + No test pattern is set. Returned frames by the camera device are\n> > > + undefined.\n> >\n> > Why do we need this ? It's not documented in the commit message. If we\n> > need this new value, its addition should be split to a separate commit,\n> > with a proper explanation.\n> >\n> \n> It was suggested by Kieran.\n> I originally used -1 for the initialized value of a test pattern mode\n> in CameraSensor.\n> -1 lets the first setTestPatternMode(Off) be executed.\n> It seems hack as -1 is invalid test pattern mode. So Kieran would like\n> to introduce this value.\n\nIn message-id:<163641141636.1410963.14556847121328252873@Monstersaurus>\nI stated:\n\n│ I'm still not sure this is needed, but I don't object to it being added\n│ if you're sure this state is required.\n│ \n│ I think the CameraSensor class should set TestPatternModeOff at init()\n│ time if there is a test pattern control available on the sensor.\n│ \n│ Then any test patterns will only be applicable if explictly requested by\n│ the application (/request).\n\nThe key point was \"If you're sure this state is required...\" which I\nstill don't believe it is?\n\nI believe my original objection was that you were initialising to '-1'\nas an otherwise arbitrary (but now defined) value.\n\nI don't think you need to do that. You can initialise the\nTestPatternMode as Off by 'setting' it (on the device) off when initialising.\n\n\n> If we move the cache of testpatternmode to ipu3 pipeline handler, this\n> may is not necessary.\n> Regards,\n> -Hiro\n> > > - name: TestPatternModeOff\n> > > value: 0\n> > > description: |\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 7C8A9BDB13\n\tfor ;\n\tWed, 24 Nov 2021 11:23:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D27656038A;\n\tWed, 24 Nov 2021 12:23:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4105360128\n\tfor ;\n\tWed, 24 Nov 2021 12:23:08 +0100 (CET)","from pendragon.ideasonboard.com\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 CADD0993;\n\tWed, 24 Nov 2021 12:23:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G1QP3bQX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637752987;\n\tbh=fIa4D6qF7x4VJIv1EdmattLVmvqZW3WNFYT5D3w0734=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=G1QP3bQXlsE7j6q3OZId6LnJwNd4s5VlKhQWo1qD3Bcrd5SzikrRHh020Hte/JAKc\n\t0n8UvZOJ+DkndTFcQVtIRSXibtm74ND65JmacvRxUyjIOfJ5w8K27YftTl24RzXc6u\n\t9OMht5bDTNgTRglv5xGOOyjuJbpaWxQQGtwV68Go=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-3-hiroh@chromium.org>\n\t\n\t","From":"Kieran Bingham ","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","Date":"Wed, 24 Nov 2021 11:23:05 +0000","Message-ID":"<163775298509.2984710.14907940678127203061@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v7 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":21204,"web_url":"https://patchwork.libcamera.org/comment/21204/","msgid":"","date":"2021-11-24T23:02:36","subject":"Re: [libcamera-devel] [PATCH v7 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":"Hello,\n\nOn Wed, Nov 24, 2021 at 11:23:05AM +0000, Kieran Bingham wrote:\n> Quoting Hirokazu Honda (2021-11-24 05:10:11)\n> > On Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart wrote:\n> > > On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:\n> > > > This adds a function to set a camera sensor driver a test pattern\n> > > > mode.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Kieran Bingham \n> > > > Reviewed-by: Laurent Pinchart \n> > >\n> > > The patch I've reviewed differs significantly from this one. Please drop\n> > > my tag in v8.\n> > >\n> > > > ---\n> > > > include/libcamera/internal/camera_sensor.h | 7 +++\n> > > > src/libcamera/camera_sensor.cpp | 60 +++++++++++++++++++---\n> > > > src/libcamera/control_ids.yaml | 5 ++\n> > > > 3 files changed, 65 insertions(+), 7 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > index edef2220..f355f323 100644\n> > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > @@ -27,6 +27,9 @@ namespace libcamera {\n> > > >\n> > > > class BayerFormat;\n> > > > class MediaEntity;\n> > > > +class Request;\n> > >\n> > > Not needed anymore.\n> > >\n> > > > +\n> > > > +struct CameraSensorProperties;\n> > > >\n> > > > class CameraSensor : protected Loggable\n> > > > {\n> > > > @@ -47,6 +50,7 @@ public:\n> > > > {\n> > > > return testPatternModes_;\n> > > > }\n> > > > + int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);\n> > > >\n> > > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > const Size &size) const;\n> > > > @@ -82,6 +86,8 @@ private:\n> > > > std::unique_ptr subdev_;\n> > > > unsigned int pad_;\n> > > >\n> > > > + const CameraSensorProperties *staticProps_;\n> > > > +\n> > > > std::string model_;\n> > > > std::string id_;\n> > > >\n> > > > @@ -89,6 +95,7 @@ private:\n> > > > std::vector mbusCodes_;\n> > > > std::vector sizes_;\n> > > > std::vector testPatternModes_;\n> > > > + controls::draft::TestPatternModeEnum testPatternMode_;\n> > > >\n> > > > Size pixelArraySize_;\n> > > > Rectangle activeArea_;\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index f0aa9f24..e1293980 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -17,6 +17,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > >\n> > > Not needed anymore either.\n> > >\n> > > >\n> > > > #include \n> > > >\n> > > > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> > > > * Once constructed the instance must be initialized with init().\n> > > > */\n> > > > CameraSensor::CameraSensor(const MediaEntity *entity)\n> > > > - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> > > > - properties_(properties::properties)\n> > > > + : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> > > > + testPatternMode_(controls::draft::TestPatternModeUnset),\n> > > > + bayerFormat_(nullptr), properties_(properties::properties)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -300,14 +302,14 @@ void CameraSensor::initVimcDefaultProperties()\n> > > >\n> > > > void CameraSensor::initStaticProperties()\n> > > > {\n> > > > - const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > > - if (!props)\n> > > > + staticProps_ = CameraSensorProperties::get(model_);\n> > > > + if (!staticProps_)\n> > > > return;\n> > > >\n> > > > /* Register the properties retrieved from the sensor database. */\n> > > > - properties_.set(properties::UnitCellSize, props->unitCellSize);\n> > > > + properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);\n> > > >\n> > > > - initTestPatternModes(props->testPatternModes);\n> > > > + initTestPatternModes(staticProps_->testPatternModes);\n> > > > }\n> > > >\n> > > > void CameraSensor::initTestPatternModes(\n> > > > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(\n> > > > {\n> > > > const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> > > > if (v4l2TestPattern == controls().end()) {\n> > > > - LOG(CameraSensor, Debug) << \"No static test pattern map for \\'\"\n> > > > + LOG(CameraSensor, Debug)\n> > > > + << \"V4L2_CID_TEST_PATTERN is not supported\";\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + if (testPatternModes.empty()) {\n> > > > + // The camera sensor supports test patterns but we don't know\n> > > > + // how to map them so this should be fixed.\n> > >\n> > > We use C-style comments.\n> > >\n> > > > + LOG(CameraSensor, Error) << \"No static test pattern map for \\'\"\n> > >\n> > > No need to escape the single quote.\n> > >\n> > > > << model() << \"\\'\";\n> > > > return;\n> > > > }\n> > > > @@ -531,6 +541,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> > > > + if (testPatternMode_ == testPatternMode)\n> > > > + return 0;\n> > > > +\n> > > > + if (!staticProps_ || testPatternModes_.empty())\n> > > > + return 0;\n> > >\n> > > Shouldn't this return an error ?\n> > >\n> > > > +\n> > > > + auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > > > + testPatternMode);\n> > > > + if (it == testPatternModes_.end()) {\n> > > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> > > > + << testPatternMode;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + int32_t index = staticProps_->testPatternModes.at(testPatternMode);\n> > > > + ControlList ctrls{ controls() };\n> > > > + ctrls.set(V4L2_CID_TEST_PATTERN, index);\n> > > > +\n> > > > + int ret = setControls(&ctrls);\n> > > > + if (ret)\n> > > > + return ret;\n> > > > +\n> > > > + testPatternMode_ = testPatternMode;\n> > >\n> > > This brings the question of where control values should be cached, to\n> > > avoid applying controls that haven't changed. I don't think this need is\n> > > limited to the CameraSensor class, so a more generic solution at the\n> > > pipeline handler level may be better. This doesn't have to be addressed\n> > > now though.\n> > >\n> > > > +\n> > > > + return 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> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 9d4638ae..083bac7b 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -639,6 +639,11 @@ controls:\n> > > > Control to select the test pattern mode. Currently identical to\n> > > > ANDROID_SENSOR_TEST_PATTERN_MODE.\n> > > > enum:\n> > > > + - name: TestPatternModeUnset\n> > > > + value: -1\n> > > > + description: |\n> > > > + No test pattern is set. Returned frames by the camera device are\n> > > > + undefined.\n> > >\n> > > Why do we need this ? It's not documented in the commit message. If we\n> > > need this new value, its addition should be split to a separate commit,\n> > > with a proper explanation.\n> > \n> > It was suggested by Kieran.\n> > I originally used -1 for the initialized value of a test pattern mode\n> > in CameraSensor.\n> > -1 lets the first setTestPatternMode(Off) be executed.\n> > It seems hack as -1 is invalid test pattern mode. So Kieran would like\n> > to introduce this value.\n> \n> In message-id:<163641141636.1410963.14556847121328252873@Monstersaurus>\n> I stated:\n> \n> │ I'm still not sure this is needed, but I don't object to it being added\n> │ if you're sure this state is required.\n> │ \n> │ I think the CameraSensor class should set TestPatternModeOff at init()\n> │ time if there is a test pattern control available on the sensor.\n> │ \n> │ Then any test patterns will only be applicable if explictly requested by\n> │ the application (/request).\n> \n> The key point was \"If you're sure this state is required...\" which I\n> still don't believe it is?\n> \n> I believe my original objection was that you were initialising to '-1'\n> as an otherwise arbitrary (but now defined) value.\n> \n> I don't think you need to do that. You can initialise the\n> TestPatternMode as Off by 'setting' it (on the device) off when initialising.\n\nThat sounds good, we should start with a known state.\n\n> > If we move the cache of testpatternmode to ipu3 pipeline handler, this\n> > may is not necessary.\n> >\n> > > > - name: TestPatternModeOff\n> > > > value: 0\n> > > > description: |","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 31DDFBDB13\n\tfor ;\n\tWed, 24 Nov 2021 23:03:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B4AC60371;\n\tThu, 25 Nov 2021 00:03:02 +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 2D5B860228\n\tfor ;\n\tThu, 25 Nov 2021 00:03:00 +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 9137E881;\n\tThu, 25 Nov 2021 00:02:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nGOMZ1WU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637794979;\n\tbh=69LAOeEeaiRpbqrBxdIJMYQMU4ghpfvJOuwmo6DRTNU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nGOMZ1WU0eUsRd1npN3zsE4F6NJGA9aEc8n2JrLKk7K1k1hnh/GDKdqMwwz2by28G\n\tNw1FQqqT5TV+ExzhT5ub0arqOBgLgSjZHMiXozpqwBqyWFr8fNQA0eM+3s1WsmaE+Y\n\t6BU5Cwmt556CaiZT74LTyPzkralgjKgNs/6Efq0M=","Date":"Thu, 25 Nov 2021 01:02:36 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-3-hiroh@chromium.org>\n\t\n\t\n\t<163775298509.2984710.14907940678127203061@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<163775298509.2984710.14907940678127203061@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v7 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\" "}}]