[{"id":20599,"web_url":"https://patchwork.libcamera.org/comment/20599/","msgid":"","date":"2021-10-28T00:36:07","subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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 Tue, Oct 26, 2021 at 07:39:13PM +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> ---\n> include/libcamera/internal/camera_sensor.h | 6 +++\n> src/libcamera/camera_sensor.cpp | 50 +++++++++++++++++++---\n> 2 files changed, 50 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index d25a1165..d08cfec5 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -26,6 +26,8 @@ namespace libcamera {\n> class BayerFormat;\n> class MediaEntity;\n> \n> +struct CameraSensorProperties;\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:\n> @@ -44,6 +46,7 @@ public:\n> \t{\n> \t\treturn testPatternModes_;\n> \t}\n> +\tint setTestPatternMode(int32_t testPatternMode);\n\nCould the function take a TestPatternModeEnum instead of an int32_t ?\nThat's the proper type of the libcamera TestPatternMode control.\n\n> \n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -78,6 +81,8 @@ private:\n> \tstd::unique_ptr subdev_;\n> \tunsigned int pad_;\n> \n> +\tconst CameraSensorProperties *props_;\n> +\n> \tstd::string model_;\n> \tstd::string id_;\n> \n> @@ -85,6 +90,7 @@ private:\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> \tstd::vector testPatternModes_;\n> +\tint32_t testPatternMode_;\n\nSame here.\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 9fdb8c09..47bb13f9 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), testPatternMode_(-1),\n\nShouldn't this be initialized to TestPatternModeOff ?\n\n> +\t bayerFormat_(nullptr), properties_(properties::properties)\n> {\n> }\n> \n> @@ -300,14 +300,16 @@ void CameraSensor::initVimcDefaultProperties()\n> \n> void CameraSensor::initStaticProperties()\n> {\n> -\tconst CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> -\tif (!props)\n> +\tprops_ = CameraSensorProperties::get(model_);\n> +\tif (!props_) {\n> +\t\tLOG(CameraSensor, Debug) << \"No properties for \" << model_;\n> \t\treturn;\n> +\t}\n> \n> \t/* Register the properties retrieved from the sensor database. */\n> -\tproperties_.set(properties::UnitCellSize, props->unitCellSize);\n> +\tproperties_.set(properties::UnitCellSize, props_->unitCellSize);\n> \n> -\tinitTestPatternModes(props->testPatternModes);\n> +\tinitTestPatternModes(props_->testPatternModes);\n> }\n> \n> void CameraSensor::initTestPatternModes(\n> @@ -531,6 +533,42 @@ Size CameraSensor::resolution() const\n> * \\return The list of test pattern modes\n> */\n> \n> +/**\n> + * \\brief Set the camera sensor a specified controls::TestPatternMode\n\n * \\brief Set the test pattern mode for the camera sensor\n\n> + * \\param[in] testPatternMode test pattern mode control value to set the camera\n\ns/test pattern/Test pattern/\n\n> + * sensor\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setTestPatternMode(int32_t testPatternMode)\n> +{\n> +\tif (testPatternMode_ == testPatternMode)\n> +\t\treturn 0;\n> +\n> +\tif (!props_) {\n> +\t\tLOG(CameraSensor, Error) << \"No property is found\";\n\ns/property is found/properties are found/\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tauto it = props_->testPatternModes.find(testPatternMode);\n> +\tif (it == props_->testPatternModes.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n\ns/mode:/mode/\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\t\t\t << testPatternMode;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tControlList ctrls{ controls() };\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, it->second);\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 D6FCDBDB1C\n\tfor ;\n\tThu, 28 Oct 2021 00:36:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32A6C60009;\n\tThu, 28 Oct 2021 02:36:34 +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 2603560006\n\tfor ;\n\tThu, 28 Oct 2021 02:36:31 +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 807D9E7;\n\tThu, 28 Oct 2021 02:36:30 +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=\"rmUxit/z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635381390;\n\tbh=GSB9qx/k2pjUS1swEGWajciCLWUdIJJJXELpIpVSfm0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rmUxit/z2ZSCE6SRc91X3VgXzhnq2pztgXlV8hTGCqqEhP5lRGX3PTRI2Ly/nHQ7X\n\tJoymN8bom7IJ13ryUqx6DuAoBiQC6qL9hqj/OVo8TV33yJ4Xbz3JIXQmJQtDTrJHcT\n\trHWjnhtDaN+qoUgX9dfFTlyLaMnFV5RqV19DweVc=","Date":"Thu, 28 Oct 2021 03:36:07 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211026103914.31449-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211026103914.31449-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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":20654,"web_url":"https://patchwork.libcamera.org/comment/20654/","msgid":"","date":"2021-11-02T02:53:34","subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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, thank you for reviewing.\n\nOn Thu, Oct 28, 2021 at 9:36 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Tue, Oct 26, 2021 at 07:39:13PM +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> > ---\n> > include/libcamera/internal/camera_sensor.h | 6 +++\n> > src/libcamera/camera_sensor.cpp | 50 +++++++++++++++++++---\n> > 2 files changed, 50 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index d25a1165..d08cfec5 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -26,6 +26,8 @@ namespace libcamera {\n> > class BayerFormat;\n> > class MediaEntity;\n> >\n> > +struct CameraSensorProperties;\n> > +\n> > class CameraSensor : protected Loggable\n> > {\n> > public:\n> > @@ -44,6 +46,7 @@ public:\n> > {\n> > return testPatternModes_;\n> > }\n> > + int setTestPatternMode(int32_t testPatternMode);\n>\n> Could the function take a TestPatternModeEnum instead of an int32_t ?\n> That's the proper type of the libcamera TestPatternMode control.\n>\n\nWe return int32_t values in testPatternModes().\nI think if we change the type here, we should change the types?\nIf yes, can I do it in a follow-up patch after submitting this patch series?\n\n> >\n> > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > const Size &size) const;\n> > @@ -78,6 +81,8 @@ private:\n> > std::unique_ptr subdev_;\n> > unsigned int pad_;\n> >\n> > + const CameraSensorProperties *props_;\n> > +\n> > std::string model_;\n> > std::string id_;\n> >\n> > @@ -85,6 +90,7 @@ private:\n> > std::vector mbusCodes_;\n> > std::vector sizes_;\n> > std::vector testPatternModes_;\n> > + int32_t testPatternMode_;\n>\n> Same here.\n>\n> >\n> > Size pixelArraySize_;\n> > Rectangle activeArea_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 9fdb8c09..47bb13f9 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> > - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> > - properties_(properties::properties)\n> > + : entity_(entity), pad_(UINT_MAX), testPatternMode_(-1),\n>\n> Shouldn't this be initialized to TestPatternModeOff ?\n>\n\nIt assumes the camera sensor's current test pattern mode is ModeOff.\nIt is not true if other test pattern mode is set to the camera sensor\nand it is closed without resetting to ModeOff.\n\nI always set camera sensor to ModeOff in initialization by setting\ntestPatternMode_ to invalid value (-1).\n\n-Hiro\n> > + bayerFormat_(nullptr), properties_(properties::properties)\n> > {\n> > }\n> >\n> > @@ -300,14 +300,16 @@ void CameraSensor::initVimcDefaultProperties()\n> >\n> > void CameraSensor::initStaticProperties()\n> > {\n> > - const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > - if (!props)\n> > + props_ = CameraSensorProperties::get(model_);\n> > + if (!props_) {\n> > + LOG(CameraSensor, Debug) << \"No properties for \" << model_;\n> > return;\n> > + }\n> >\n> > /* Register the properties retrieved from the sensor database. */\n> > - properties_.set(properties::UnitCellSize, props->unitCellSize);\n> > + properties_.set(properties::UnitCellSize, props_->unitCellSize);\n> >\n> > - initTestPatternModes(props->testPatternModes);\n> > + initTestPatternModes(props_->testPatternModes);\n> > }\n> >\n> > void CameraSensor::initTestPatternModes(\n> > @@ -531,6 +533,42 @@ Size CameraSensor::resolution() const\n> > * \\return The list of test pattern modes\n> > */\n> >\n> > +/**\n> > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n>\n> * \\brief Set the test pattern mode for the camera sensor\n>\n> > + * \\param[in] testPatternMode test pattern mode control value to set the camera\n>\n> s/test pattern/Test pattern/\n>\n> > + * sensor\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraSensor::setTestPatternMode(int32_t testPatternMode)\n> > +{\n> > + if (testPatternMode_ == testPatternMode)\n> > + return 0;\n> > +\n> > + if (!props_) {\n> > + LOG(CameraSensor, Error) << \"No property is found\";\n>\n> s/property is found/properties are found/\n>\n> > + return -EINVAL;\n> > + }\n> > +\n> > + auto it = props_->testPatternModes.find(testPatternMode);\n> > + if (it == props_->testPatternModes.end()) {\n> > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n>\n> s/mode:/mode/\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > + << testPatternMode;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + ControlList ctrls{ controls() };\n> > + ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > +\n> > + int ret = setControls(&ctrls);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + testPatternMode_ = testPatternMode;\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>\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 AEA56BF415\n\tfor ;\n\tTue, 2 Nov 2021 02:53:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA80D60122;\n\tTue, 2 Nov 2021 03:53:46 +0100 (CET)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66B7660122\n\tfor ;\n\tTue, 2 Nov 2021 03:53:45 +0100 (CET)","by mail-ed1-x535.google.com with SMTP id g10so70539200edj.1\n\tfor ;\n\tMon, 01 Nov 2021 19:53:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"gH9dESw3\"; 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=0J4EYssIoAX93/vIhbjf62+XiSsIXlx3HCF9/I+VPQ0=;\n\tb=gH9dESw3cXI14YwilxjdrHb3hXHmj5kNgm9/Uh21/Zs6gamcsMmMNZVzg0amjO1Qpz\n\tNDeh4wBvgqe85zckpEGM1T/1I1KhlKm0mGY6c4Zb4jnmqe0hQQkYg8HPdBEJ6hTmn3vs\n\tDd8R2qscPiZmVOUOo25kdLIPp11lR372KDpP0=","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=0J4EYssIoAX93/vIhbjf62+XiSsIXlx3HCF9/I+VPQ0=;\n\tb=ZtlshfG7o5ph5oxM61u7mnCWHc8SqnBTbxlN/52MxDy+nJLMSemTNlG+Wbd7qQ/PrK\n\tXrgxCQD09CZ7GoT4+w+qYjdn4QZfM1BVY6tQXfL2WHITbPf/kkWAWOGV73OI904d/JxK\n\tNGOYXPM8ZcBbXz5Ln7Y3YkdysCjv8/FKJBwShcoNZGXL2o/kL1RjBHzNAAYWcYMZFucG\n\tr79x/pX+YTU/91kSeQV0MT5evA6VdLlak7abu63VO1RY9w81/jQYLWS0/jtQ/ZZT9g4V\n\t4DVY6/V2m6Yy5/jjbczaTMLY1YfQ9fu7CCPJ4nTaO4nOaIa62tbnGIYTnZVaN5GIKdSS\n\t6JRg==","X-Gm-Message-State":"AOAM532A7iZC7jK/Cyik5PaYSiU/qgvw1FP75MNrUCPiONCkmcB4O02d\n\t1pqyDiZP1jaL2Pxd9ZAVhDZ7vEEqbtGMckEslYCDCa5T+/s=","X-Google-Smtp-Source":"ABdhPJxgCTJv3ixqv67ZPeuv+4uvlfsD1CXWitawjMpNMKBBI2eOd2HHHZUUTpDJOHudTE2n9WD68C6nR6exK2n0OGQ=","X-Received":"by 2002:a17:906:1815:: with SMTP id\n\tv21mr42276375eje.218.1635821624878; \n\tMon, 01 Nov 2021 19:53:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20211026103914.31449-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 2 Nov 2021 11:53:34 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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":20656,"web_url":"https://patchwork.libcamera.org/comment/20656/","msgid":"<163585038386.1097798.12002874804035302476@Monstersaurus>","date":"2021-11-02T10:53:03","subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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-02 02:53:34)\n> Hi Laurent, thank you for reviewing.\n> \n> On Thu, Oct 28, 2021 at 9:36 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Oct 26, 2021 at 07:39:13PM +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> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 6 +++\n> > > src/libcamera/camera_sensor.cpp | 50 +++++++++++++++++++---\n> > > 2 files changed, 50 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index d25a1165..d08cfec5 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -26,6 +26,8 @@ namespace libcamera {\n> > > class BayerFormat;\n> > > class MediaEntity;\n> > >\n> > > +struct CameraSensorProperties;\n> > > +\n> > > class CameraSensor : protected Loggable\n> > > {\n> > > public:\n> > > @@ -44,6 +46,7 @@ public:\n> > > {\n> > > return testPatternModes_;\n> > > }\n> > > + int setTestPatternMode(int32_t testPatternMode);\n> >\n> > Could the function take a TestPatternModeEnum instead of an int32_t ?\n> > That's the proper type of the libcamera TestPatternMode control.\n> >\n> \n> We return int32_t values in testPatternModes().\n> I think if we change the type here, we should change the types?\n> If yes, can I do it in a follow-up patch after submitting this patch series?\n> \n> > >\n> > > V4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > const Size &size) const;\n> > > @@ -78,6 +81,8 @@ private:\n> > > std::unique_ptr subdev_;\n> > > unsigned int pad_;\n> > >\n> > > + const CameraSensorProperties *props_;\n> > > +\n> > > std::string model_;\n> > > std::string id_;\n> > >\n> > > @@ -85,6 +90,7 @@ private:\n> > > std::vector mbusCodes_;\n> > > std::vector sizes_;\n> > > std::vector testPatternModes_;\n> > > + int32_t testPatternMode_;\n> >\n> > Same here.\n> >\n> > >\n> > > Size pixelArraySize_;\n> > > Rectangle activeArea_;\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 9fdb8c09..47bb13f9 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> > > - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> > > - properties_(properties::properties)\n> > > + : entity_(entity), pad_(UINT_MAX), testPatternMode_(-1),\n> >\n> > Shouldn't this be initialized to TestPatternModeOff ?\n> >\n> \n> It assumes the camera sensor's current test pattern mode is ModeOff.\n> It is not true if other test pattern mode is set to the camera sensor\n> and it is closed without resetting to ModeOff.\n> \n> I always set camera sensor to ModeOff in initialization by setting\n> testPatternMode_ to invalid value (-1).\n \n\nThis throws away the ability to use the type information though.\n\nWe could add TestPatternModeUnset to initialise it to 0, and keep the\ntype? Or is that overkill?\n\n\n> -Hiro\n> > > + bayerFormat_(nullptr), properties_(properties::properties)\n> > > {\n> > > }\n> > >\n> > > @@ -300,14 +300,16 @@ void CameraSensor::initVimcDefaultProperties()\n> > >\n> > > void CameraSensor::initStaticProperties()\n> > > {\n> > > - const CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> > > - if (!props)\n> > > + props_ = CameraSensorProperties::get(model_);\n> > > + if (!props_) {\n> > > + LOG(CameraSensor, Debug) << \"No properties for \" << model_;\n> > > return;\n> > > + }\n> > >\n> > > /* Register the properties retrieved from the sensor database. */\n> > > - properties_.set(properties::UnitCellSize, props->unitCellSize);\n> > > + properties_.set(properties::UnitCellSize, props_->unitCellSize);\n> > >\n> > > - initTestPatternModes(props->testPatternModes);\n> > > + initTestPatternModes(props_->testPatternModes);\n> > > }\n> > >\n> > > void CameraSensor::initTestPatternModes(\n> > > @@ -531,6 +533,42 @@ Size CameraSensor::resolution() const\n> > > * \\return The list of test pattern modes\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Set the camera sensor a specified controls::TestPatternMode\n> >\n> > * \\brief Set the test pattern mode for the camera sensor\n> >\n> > > + * \\param[in] testPatternMode test pattern mode control value to set the camera\n> >\n> > s/test pattern/Test pattern/\n> >\n> > > + * sensor\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int CameraSensor::setTestPatternMode(int32_t testPatternMode)\n> > > +{\n> > > + if (testPatternMode_ == testPatternMode)\n> > > + return 0;\n> > > +\n> > > + if (!props_) {\n> > > + LOG(CameraSensor, Error) << \"No property is found\";\n> >\n> > s/property is found/properties are found/\n> >\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + auto it = props_->testPatternModes.find(testPatternMode);\n> > > + if (it == props_->testPatternModes.end()) {\n> > > + LOG(CameraSensor, Error) << \"Unsupported test pattern mode: \"\n> >\n> > s/mode:/mode/\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > + << testPatternMode;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + ControlList ctrls{ controls() };\n> > > + ctrls.set(V4L2_CID_TEST_PATTERN, it->second);\n> > > +\n> > > + int ret = setControls(&ctrls);\n> > > + if (ret)\n> > > + return ret;\n> > > +\n> > > + testPatternMode_ = testPatternMode;\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> >\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 CE7E0BF415\n\tfor ;\n\tTue, 2 Nov 2021 10:53:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0758A60326;\n\tTue, 2 Nov 2021 11:53:09 +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 40066600B6\n\tfor ;\n\tTue, 2 Nov 2021 11:53:07 +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 BD2F63E5;\n\tTue, 2 Nov 2021 11:53:06 +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=\"fkmVFSN7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635850386;\n\tbh=sntcOaSMndg1fwowoNAeQr36Jdp3WJ/+e5L8BbhhD2o=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=fkmVFSN7zkW1oRcAqzG864ygwZWflk/jQUACQ8BK4lcNidQXvHi0i9xicVnvnLHev\n\tSWMMEJRdcjKOqLU2pjKKBDAT3gVYcUmn/0MI9aOvxNt5WhmmWIr/oFq71KfshwW/d5\n\tAhRthY8UT3Skt02JSH433NQTnNE65gIpm7gryqFY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211026103914.31449-1-hiroh@chromium.org>\n\t\n\t","From":"Kieran Bingham ","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","Date":"Tue, 02 Nov 2021 10:53:03 +0000","Message-ID":"<163585038386.1097798.12002874804035302476@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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":20660,"web_url":"https://patchwork.libcamera.org/comment/20660/","msgid":"<20211102115205.o6a5eri2huogvl6q@uno.localdomain>","date":"2021-11-02T11:52:05","subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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 Tue, Oct 26, 2021 at 07:39:13PM +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> ---\n> include/libcamera/internal/camera_sensor.h | 6 +++\n> src/libcamera/camera_sensor.cpp | 50 +++++++++++++++++++---\n> 2 files changed, 50 insertions(+), 6 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index d25a1165..d08cfec5 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -26,6 +26,8 @@ namespace libcamera {\n> class BayerFormat;\n> class MediaEntity;\n>\n> +struct CameraSensorProperties;\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:\n> @@ -44,6 +46,7 @@ public:\n> \t{\n> \t\treturn testPatternModes_;\n> \t}\n> +\tint setTestPatternMode(int32_t testPatternMode);\n>\n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -78,6 +81,8 @@ private:\n> \tstd::unique_ptr subdev_;\n> \tunsigned int pad_;\n>\n> +\tconst CameraSensorProperties *props_;\n> +\n> \tstd::string model_;\n> \tstd::string id_;\n>\n> @@ -85,6 +90,7 @@ private:\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> \tstd::vector testPatternModes_;\n> +\tint32_t 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 9fdb8c09..47bb13f9 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), testPatternMode_(-1),\n> +\t bayerFormat_(nullptr), properties_(properties::properties)\n\n props_(nullptr)\n\nAlso, we now have props_ and properties_, we should find a better\nname.\n\nI would s/props_/staticProps_/ but there might be better alternatives\n\n> {\n> }\n>\n> @@ -300,14 +300,16 @@ void CameraSensor::initVimcDefaultProperties()\n>\n> void CameraSensor::initStaticProperties()\n> {\n> -\tconst CameraSensorProperties *props = CameraSensorProperties::get(model_);\n> -\tif (!props)\n> +\tprops_ = CameraSensorProperties::get(model_);\n> +\tif (!props_) {\n> +\t\tLOG(CameraSensor, Debug) << \"No properties for \" << model_;\n\nThe failed lookup will already print a rather verbose warning\n\n\t\tLOG(CameraSensorProperties, Warning)\n\t\t\t<< \"No static properties available for '\" << sensor << \"'\";\n\t\tLOG(CameraSensorProperties, Warning)\n\t\t\t<< \"Please consider updating the camera sensor properties database\";\n\nI would drop it from here\n\n\n> \t\treturn;\n> +\t}\n>\n> \t/* Register the properties retrieved from the sensor database. */\n> -\tproperties_.set(properties::UnitCellSize, props->unitCellSize);\n> +\tproperties_.set(properties::UnitCellSize, props_->unitCellSize);\n>\n> -\tinitTestPatternModes(props->testPatternModes);\n> +\tinitTestPatternModes(props_->testPatternModes);\n> }\n>\n> void CameraSensor::initTestPatternModes(\n> @@ -531,6 +533,42 @@ Size CameraSensor::resolution() const\n> * \\return The list of test pattern modes\n> */\n>\n> +/**\n> + * \\brief Set the camera sensor a specified controls::TestPatternMode\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(int32_t testPatternMode)\n> +{\n> +\tif (testPatternMode_ == testPatternMode)\n> +\t\treturn 0;\n> +\n> +\tif (!props_) {\n> +\t\tLOG(CameraSensor, Error) << \"No property is found\";\n> +\t\treturn -EINVAL;\n> +\t}\n\nI'm tempted to say we should either if (!props_) { return 0; } or\nASSERT(props_); as returning an error can easily get not noticed, see\nin 2/2 with the un-checked call:\n cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);\n\nAlso a bit unrelated but since I just noticed that:\nthe test pattern mode initialization seems to require a small fix. We have:\n\nvoid CameraSensor::initStaticProperties()\n{\n\tprops_ = CameraSensorProperties::get(model_);\n\n ....\n\n\tinitTestPatternModes(props_->testPatternModes);\n}\n\nvoid CameraSensor::initTestPatternModes(\n\tconst std::map &testPatternModes)\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\t\t\t\t << model() << \"\\'\";\n\t\treturn;\n\t}\n}\n\nThe error message in CameraSensor::initTestPatternModes() says\n\"No static test pattern map for << model()\nwhile what is actually missing is the V4L2 control in the driver.\n\nShould we check that props->testPatternModes is not empty and bail out\nfrom CameraSensor::initTestPatternModes() like we do if the v4l2\ncontrol is not found (and fix the error message while at it) so that\nonce we get to setTestPatternMode() we'll have an empty testPatternModes_\nand do not risk to set an unsupported value ?\n\n> +\n> +\tauto it = props_->testPatternModes.find(testPatternMode);\n\nWe build testPatternModes_ in initTestPatternModes() for the purpose\nof matching the driver reported indexes and the entries in the sensor\ndatabase. I would use the vector to verify the test pattern is\nsupported instead of the raw map. If in the un-likely case the driver\ndrops support for a test pattern, we're safe (at the expense of an\nadditional lookup in a reasonably small vector). Also, in the case the\ncamera properties do not contain the test patterns map and\nyou take my above suggestion in of bailing out earlier in\ninitTestPatternModes() because of that, we'll bail out here by simply\nveryfing the vector contains the requested test pattern.\n\nI think you can reduce error check in this function to\n\n auto it = std::find(testPatternModes_, testPatternMode);\n if (it == testPatternModes_.end())\n Error or Fatal ?\n\nand the simply access the static map (which is now guaranteed to be\npresent and have an entry for testPatternMode)\n\n int32_t index = props_->testPatternModes.find(testPatternMode)->second;\n ControlList ctrls{ controls() };\n\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n\n ...\n\n\n> +\tif (it == props_->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> +\tControlList ctrls{ controls() };\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, it->second);\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.33.0.1079.g6e70778dc9-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 7F605BF415\n\tfor ;\n\tTue, 2 Nov 2021 11:51:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED7B4600C2;\n\tTue, 2 Nov 2021 12:51:15 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85AC1600B8\n\tfor ;\n\tTue, 2 Nov 2021 12:51:14 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 06DEB1BF208;\n\tTue, 2 Nov 2021 11:51:13 +0000 (UTC)"],"Date":"Tue, 2 Nov 2021 12:52:05 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211102115205.o6a5eri2huogvl6q@uno.localdomain>","References":"<20211026103914.31449-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211026103914.31449-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] 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\" "}}]