[{"id":11773,"web_url":"https://patchwork.libcamera.org/comment/11773/","msgid":"<20200802182655.GD23801@pendragon.ideasonboard.com>","date":"2020-08-02T18:26:55","subject":"Re: [libcamera-devel] [PATCH v5 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Jul 29, 2020 at 01:50:52PM +0200, Niklas Söderlund wrote:\n> Generate a constant and unique string ID for the sensor. The ID is\n> generated from information from the firmware description of the camera\n> image sensor. The ID is unique and persistent across reboots of the\n> system.\n> \n> For OF based systems the ID is the full path of the sensors in the\n> device tree description. For ACPI based systems the ID is the ACPI\n> firmware nodes path. Both ID sources are guaranteed to be unique and\n> persistent as long as the firmware of the system is not changed.\n> \n> A special case is needed to deal with the VIMC pipeline that implements\n> a virtual pipeline that is not backed by any hardware device and is\n> therefore not described in the device firmware.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> * Changes since v4\n> - Fix spelling.\n> \n> * Changes since v3\n> - Update commit message.\n> - Add description of how ID are generated to comment.\n> ---\n> include/libcamera/internal/camera_sensor.h | 4 +\n> src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++++++++\n> 2 files changed, 98 insertions(+)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 06c8292ca30129de..e0d2d9f63b47c2fe 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -47,6 +47,7 @@ public:\n> \tint init();\n> \n> \tconst std::string &model() const { return model_; }\n> +\tconst std::string &id() const { return id_; }\n> \tconst MediaEntity *entity() const { return entity_; }\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tconst std::vector &sizes() const { return sizes_; }\n> @@ -67,11 +68,14 @@ protected:\n> \tstd::string logPrefix() const override;\n> \n> private:\n> +\tint generateID();\n> +\n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> \tunsigned int pad_;\n> \n> \tstd::string model_;\n> +\tstd::string id_;\n> \n> \tV4L2Subdevice::Formats formats_;\n> \tSize resolution_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 350f49accad99c7b..6dc616945dad5ef1 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -9,10 +9,12 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -204,6 +206,11 @@ int CameraSensor::init()\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> +\t/* Generate a unique ID for the sensor. */\n> +\tret = generateID();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \t/* Retrieve and store the camera sensor properties. */\n> \tconst ControlInfoMap &controls = subdev_->controls();\n> \tint32_t propertyValue;\n> @@ -283,6 +290,26 @@ int CameraSensor::init()\n> * \\return The sensor model name\n> */\n> \n> +/**\n> + * \\fn CameraSensor::id()\n> + * \\brief Retrieve the sensor ID\n> + *\n> + * The sensor ID is a free-formed string that uniquely identifies the sensor in\n> + * the system. The ID is persistent between different instances of libcamera and\n> + * between resets of the system.\n> + *\n> + * For OF based systems the ID is the full path of the sensors in the device\n> + * tree description. For ACPI based systems the ID is the ACPI firmware nodes\n> + * path. Both ID sources are guaranteed to be unique and persistent as long as\n> + * the firmware of the system is not changed.\n> + *\n> + * A special case is needed to deal with the VIMC pipeline that implements a\n> + * virtual pipeline that is not backed by any hardware device and is therefore\n> + * not described in the device firmware.\n> + *\n> + * \\return The sensor ID\n> + */\n> +\n> /**\n> * \\fn CameraSensor::entity()\n> * \\brief Retrieve the sensor media entity\n> @@ -541,4 +568,71 @@ std::string CameraSensor::logPrefix() const\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n> \n> +int CameraSensor::generateID()\n> +{\n> +\tstd::string path, devPath = subdev_->devicePath();\n\nWe usually split declarations on multiple lines.\n\n> +\tstruct stat statbuf;\n> +\n> +\t/* Try to generate ID from OF device tree path. */\n> +\tpath = devPath + \"/of_node\";\n> +\tif (stat(path.c_str(), &statbuf) == 0) {\n\nYou can use\n\n\tif (!File::exists(path)) {\n\n> +\t\tchar ofPath[PATH_MAX];\n> +\n> +\t\tif (!realpath(path.c_str(), ofPath)) {\n\nAs mentioned in the review of 1/5, should we use realpath with nullptr\nas the second argument ? Or maybe just use readlink() with PATH_MAX ?\n\n> +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor OF based ID\";\n\ns/OF based/OF-based/\n\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tid_ = ofPath;\n> +\t\tconst std::string dropStr = \"/sys/firmware/devicetree/\";\n\nstatic const ?\n\nSame comments for the other options below.\n\n> +\t\tif (id_.find(dropStr) == 0)\n> +\t\t\tid_.erase(0, dropStr.length());\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* Try to generate ID from ACPI path. */\n> +\tpath = devPath + \"/firmware_node/path\";\n> +\tif (stat(path.c_str(), &statbuf) == 0) {\n> +\t\tstd::ifstream file(path.c_str());\n> +\n> +\t\tif (!file.is_open()) {\n> +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor ACPI based ID\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tstd::getline(file, id_);\n> +\t\tfile.close();\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/*\n> +\t * VIMC is a virtual video pipeline not backed hardware and has no OF\n> +\t * or ACPI firmware nodes. Handle this pipeline as a special case and\n> +\t * generate IDs based on the sensor model.\n> +\t */\n> +\tpath = devPath + \"/modalias\";\n> +\tif (stat(path.c_str(), &statbuf) == 0) {\n> +\t\tstd::ifstream file(path.c_str());\n> +\n> +\t\tif (!file.is_open()) {\n> +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor ACPI based ID\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tstd::string modalias;\n> +\t\tstd::getline(file, modalias);\n> +\t\tfile.close();\n> +\n> +\t\tif (modalias == \"platform:vimc\") {\n> +\t\t\tid_ = \"VIMC \" + model();\n> +\t\t\treturn 0;\n> +\t\t}\n\nShould we make this case more generic with the following (pseudo-code) ?\n\n\tid_ = basename(readlink(devPath + \"/device\")) + \"/\" + model_;\n\n> +\t}\n> +\n> +\tLOG(CameraSensor, Error) << \"Unknown sensor device type, can not generate ID\";\n> +\treturn -EINVAL;\n> +}\n> +\n> } /* namespace libcamera */","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 B4398BD86F\n\tfor ;\n\tSun, 2 Aug 2020 18:27:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4312960999;\n\tSun, 2 Aug 2020 20:27:07 +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 523CF60393\n\tfor ;\n\tSun, 2 Aug 2020 20:27:06 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B05EA296;\n\tSun, 2 Aug 2020 20:27:05 +0200 (CEST)"],"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=\"am4R5J/n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596392825;\n\tbh=ncyjusvMWfLJPBBkQQ9T7TMybPsy0zD0s7+fjyScSQg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=am4R5J/n4w0DpZ5aSzyFAv2f2K3twEThprZq9aOFm+3pyX6VryNUzDym8nkz4i/7q\n\tB8iI43dES0XdugRSrSPGV/RbBOo5bexGaay1n2eojS+tkEIwB2+CNu2K6XLZiGbCA7\n\txXR368zffpzKDrlizAtUSsD60Iw0x5PqhEvSh62M=","Date":"Sun, 2 Aug 2020 21:26:55 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200802182655.GD23801@pendragon.ideasonboard.com>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729115055.3840110-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v5 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11778,"web_url":"https://patchwork.libcamera.org/comment/11778/","msgid":"<20200802214637.GA1945056@oden.dyn.berto.se>","date":"2020-08-02T21:46:37","subject":"Re: [libcamera-devel] [PATCH v5 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-08-02 21:26:55 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Wed, Jul 29, 2020 at 01:50:52PM +0200, Niklas Söderlund wrote:\n> > Generate a constant and unique string ID for the sensor. The ID is\n> > generated from information from the firmware description of the camera\n> > image sensor. The ID is unique and persistent across reboots of the\n> > system.\n> > \n> > For OF based systems the ID is the full path of the sensors in the\n> > device tree description. For ACPI based systems the ID is the ACPI\n> > firmware nodes path. Both ID sources are guaranteed to be unique and\n> > persistent as long as the firmware of the system is not changed.\n> > \n> > A special case is needed to deal with the VIMC pipeline that implements\n> > a virtual pipeline that is not backed by any hardware device and is\n> > therefore not described in the device firmware.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > * Changes since v4\n> > - Fix spelling.\n> > \n> > * Changes since v3\n> > - Update commit message.\n> > - Add description of how ID are generated to comment.\n> > ---\n> > include/libcamera/internal/camera_sensor.h | 4 +\n> > src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++++++++\n> > 2 files changed, 98 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 06c8292ca30129de..e0d2d9f63b47c2fe 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -47,6 +47,7 @@ public:\n> > \tint init();\n> > \n> > \tconst std::string &model() const { return model_; }\n> > +\tconst std::string &id() const { return id_; }\n> > \tconst MediaEntity *entity() const { return entity_; }\n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > \tconst std::vector &sizes() const { return sizes_; }\n> > @@ -67,11 +68,14 @@ protected:\n> > \tstd::string logPrefix() const override;\n> > \n> > private:\n> > +\tint generateID();\n> > +\n> > \tconst MediaEntity *entity_;\n> > \tstd::unique_ptr subdev_;\n> > \tunsigned int pad_;\n> > \n> > \tstd::string model_;\n> > +\tstd::string id_;\n> > \n> > \tV4L2Subdevice::Formats formats_;\n> > \tSize resolution_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 350f49accad99c7b..6dc616945dad5ef1 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -9,10 +9,12 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \n> > \n> > @@ -204,6 +206,11 @@ int CameraSensor::init()\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > +\t/* Generate a unique ID for the sensor. */\n> > +\tret = generateID();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \t/* Retrieve and store the camera sensor properties. */\n> > \tconst ControlInfoMap &controls = subdev_->controls();\n> > \tint32_t propertyValue;\n> > @@ -283,6 +290,26 @@ int CameraSensor::init()\n> > * \\return The sensor model name\n> > */\n> > \n> > +/**\n> > + * \\fn CameraSensor::id()\n> > + * \\brief Retrieve the sensor ID\n> > + *\n> > + * The sensor ID is a free-formed string that uniquely identifies the sensor in\n> > + * the system. The ID is persistent between different instances of libcamera and\n> > + * between resets of the system.\n> > + *\n> > + * For OF based systems the ID is the full path of the sensors in the device\n> > + * tree description. For ACPI based systems the ID is the ACPI firmware nodes\n> > + * path. Both ID sources are guaranteed to be unique and persistent as long as\n> > + * the firmware of the system is not changed.\n> > + *\n> > + * A special case is needed to deal with the VIMC pipeline that implements a\n> > + * virtual pipeline that is not backed by any hardware device and is therefore\n> > + * not described in the device firmware.\n> > + *\n> > + * \\return The sensor ID\n> > + */\n> > +\n> > /**\n> > * \\fn CameraSensor::entity()\n> > * \\brief Retrieve the sensor media entity\n> > @@ -541,4 +568,71 @@ std::string CameraSensor::logPrefix() const\n> > \treturn \"'\" + entity_->name() + \"'\";\n> > }\n> > \n> > +int CameraSensor::generateID()\n> > +{\n> > +\tstd::string path, devPath = subdev_->devicePath();\n> \n> We usually split declarations on multiple lines.\n> \n> > +\tstruct stat statbuf;\n> > +\n> > +\t/* Try to generate ID from OF device tree path. */\n> > +\tpath = devPath + \"/of_node\";\n> > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> \n> You can use\n> \n> \tif (!File::exists(path)) {\n> \n> > +\t\tchar ofPath[PATH_MAX];\n> > +\n> > +\t\tif (!realpath(path.c_str(), ofPath)) {\n> \n> As mentioned in the review of 1/5, should we use realpath with nullptr\n> as the second argument ? Or maybe just use readlink() with PATH_MAX ?\n> \n> > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor OF based ID\";\n> \n> s/OF based/OF-based/\n> \n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tid_ = ofPath;\n> > +\t\tconst std::string dropStr = \"/sys/firmware/devicetree/\";\n> \n> static const ?\n> \n> Same comments for the other options below.\n> \n> > +\t\tif (id_.find(dropStr) == 0)\n> > +\t\t\tid_.erase(0, dropStr.length());\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\t/* Try to generate ID from ACPI path. */\n> > +\tpath = devPath + \"/firmware_node/path\";\n> > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> > +\t\tstd::ifstream file(path.c_str());\n> > +\n> > +\t\tif (!file.is_open()) {\n> > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor ACPI based ID\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tstd::getline(file, id_);\n> > +\t\tfile.close();\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * VIMC is a virtual video pipeline not backed hardware and has no OF\n> > +\t * or ACPI firmware nodes. Handle this pipeline as a special case and\n> > +\t * generate IDs based on the sensor model.\n> > +\t */\n> > +\tpath = devPath + \"/modalias\";\n> > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> > +\t\tstd::ifstream file(path.c_str());\n> > +\n> > +\t\tif (!file.is_open()) {\n> > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor ACPI based ID\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tstd::string modalias;\n> > +\t\tstd::getline(file, modalias);\n> > +\t\tfile.close();\n> > +\n> > +\t\tif (modalias == \"platform:vimc\") {\n> > +\t\t\tid_ = \"VIMC \" + model();\n> > +\t\t\treturn 0;\n> > +\t\t}\n> \n> Should we make this case more generic with the following (pseudo-code) ?\n> \n> \tid_ = basename(readlink(devPath + \"/device\")) + \"/\" + model_;\n\nI don't think so, at least not for now. The only valid use-case to reach \nthis code is for the VIMC pipeline handler (and later perhaps VIVID). I \nwould prefers keeping it strict for now and loosen it as we add \npipelines we can test and make sure the id is stable.\n\n> \n> > +\t}\n> > +\n> > +\tLOG(CameraSensor, Error) << \"Unknown sensor device type, can not generate ID\";\n> > +\treturn -EINVAL;\n> > +}\n> > +\n> > } /* namespace libcamera */\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 89543BD87A\n\tfor ;\n\tSun, 2 Aug 2020 21:46:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 183BF60999;\n\tSun, 2 Aug 2020 23:46:41 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A57760393\n\tfor ;\n\tSun, 2 Aug 2020 23:46:40 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id t23so14098168ljc.3\n\tfor ;\n\tSun, 02 Aug 2020 14:46:39 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj6sm3859158ljc.18.2020.08.02.14.46.38\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 02 Aug 2020 14:46:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"MuOylUMW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=2rtDzdxOSNXy742wpubQ2tC0Y63r8UvmRgUevBPJ+/o=;\n\tb=MuOylUMWKjnGfYLFP/vETl+AkSJbfkzSkDNM8pIsEMiyom/5dKATwUS43oDUUckI3x\n\ta4D3sTxs/Ds7IYhWz0s+JY0DzwUMpPg7n8/MNX1eKVEEmESgLS7kX2/AB9kBTaEDw5cG\n\tjGwnCtd8Kl2rdT4FH2p0vcM8rnrJqDoB7TukyCQTaiF2zvOYXusszL23ZsTeks7+Es/R\n\tmPzpx66FjiLSlF9V0uuzNGS1GOVC8d4yiN1ep9waQpWrrbXhjgfig6zqkQOcPwn78fJq\n\tYyOw5yauyB2v2ZeeuAgXaUFwhdp+gZS5Zm2YtJQmp4z9yjorCbnI0aL7yw+5WkzW4m4H\n\tja6g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=2rtDzdxOSNXy742wpubQ2tC0Y63r8UvmRgUevBPJ+/o=;\n\tb=RFMa2as7gQzUbutj/5NnioLo+jl522vQF7+UMx3eG4lbxpAhxtlxY+ioz7jWN0BN8K\n\t5OHBaAb8vxJdMt+QlExVLlYTvMl9UNtERQjnix9O/l860gshU0DqO/FdVjgfqENaYmDT\n\tkkbX/ybOtNNA+1FXvxiWWAvpwuFbfrcebcNpZgkvPgWnGZ34qP+kEUkVGz9JqND2AN8M\n\tl3pBEPasqqjRB5XcwMnPRpIscKwmiu7lEPmEPEmJu210gy7sB89BKwD1NFK+wIWCYEqI\n\tbXPwW4KawjN7VWm5qZtGakxqpppNg9x0M8ic6LxPfq8H/5K0ruDdYCiFpdrWJZRrUVcs\n\tVNbA==","X-Gm-Message-State":"AOAM533kU7pi0A3pT3GkX63LA3ivIiotEkgaQigvSuMBklogiWhP3TTo\n\t0FipT32TQMNgITCfxACX4B/FyTwxlaI=","X-Google-Smtp-Source":"ABdhPJyq2NJ7JPbkv+jLK3a9DzFjMpOj3jqI3omk5KXLc/l/SlpKZvtzfsQ6qwodnlUWUYPHTnnGRg==","X-Received":"by 2002:a2e:859a:: with SMTP id\n\tb26mr6878011lji.241.1596404799282; \n\tSun, 02 Aug 2020 14:46:39 -0700 (PDT)","Date":"Sun, 2 Aug 2020 23:46:37 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200802214637.GA1945056@oden.dyn.berto.se>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-3-niklas.soderlund@ragnatech.se>\n\t<20200802182655.GD23801@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200802182655.GD23801@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11779,"web_url":"https://patchwork.libcamera.org/comment/11779/","msgid":"<20200802215045.GH23801@pendragon.ideasonboard.com>","date":"2020-08-02T21:50:45","subject":"Re: [libcamera-devel] [PATCH v5 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sun, Aug 02, 2020 at 11:46:37PM +0200, Niklas Söderlund wrote:\n> On 2020-08-02 21:26:55 +0300, Laurent Pinchart wrote:\n> > On Wed, Jul 29, 2020 at 01:50:52PM +0200, Niklas Söderlund wrote:\n> > > Generate a constant and unique string ID for the sensor. The ID is\n> > > generated from information from the firmware description of the camera\n> > > image sensor. The ID is unique and persistent across reboots of the\n> > > system.\n> > > \n> > > For OF based systems the ID is the full path of the sensors in the\n> > > device tree description. For ACPI based systems the ID is the ACPI\n> > > firmware nodes path. Both ID sources are guaranteed to be unique and\n> > > persistent as long as the firmware of the system is not changed.\n> > > \n> > > A special case is needed to deal with the VIMC pipeline that implements\n> > > a virtual pipeline that is not backed by any hardware device and is\n> > > therefore not described in the device firmware.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > Reviewed-by: Jacopo Mondi \n> > > Reviewed-by: Kieran Bingham \n> > > ---\n> > > * Changes since v4\n> > > - Fix spelling.\n> > > \n> > > * Changes since v3\n> > > - Update commit message.\n> > > - Add description of how ID are generated to comment.\n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 4 +\n> > > src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++++++++\n> > > 2 files changed, 98 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index 06c8292ca30129de..e0d2d9f63b47c2fe 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -47,6 +47,7 @@ public:\n> > > \tint init();\n> > > \n> > > \tconst std::string &model() const { return model_; }\n> > > +\tconst std::string &id() const { return id_; }\n> > > \tconst MediaEntity *entity() const { return entity_; }\n> > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > \tconst std::vector &sizes() const { return sizes_; }\n> > > @@ -67,11 +68,14 @@ protected:\n> > > \tstd::string logPrefix() const override;\n> > > \n> > > private:\n> > > +\tint generateID();\n> > > +\n> > > \tconst MediaEntity *entity_;\n> > > \tstd::unique_ptr subdev_;\n> > > \tunsigned int pad_;\n> > > \n> > > \tstd::string model_;\n> > > +\tstd::string id_;\n> > > \n> > > \tV4L2Subdevice::Formats formats_;\n> > > \tSize resolution_;\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 350f49accad99c7b..6dc616945dad5ef1 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -9,10 +9,12 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > \n> > > #include \n> > > \n> > > @@ -204,6 +206,11 @@ int CameraSensor::init()\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > > \n> > > +\t/* Generate a unique ID for the sensor. */\n> > > +\tret = generateID();\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > \t/* Retrieve and store the camera sensor properties. */\n> > > \tconst ControlInfoMap &controls = subdev_->controls();\n> > > \tint32_t propertyValue;\n> > > @@ -283,6 +290,26 @@ int CameraSensor::init()\n> > > * \\return The sensor model name\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn CameraSensor::id()\n> > > + * \\brief Retrieve the sensor ID\n> > > + *\n> > > + * The sensor ID is a free-formed string that uniquely identifies the sensor in\n> > > + * the system. The ID is persistent between different instances of libcamera and\n> > > + * between resets of the system.\n> > > + *\n> > > + * For OF based systems the ID is the full path of the sensors in the device\n> > > + * tree description. For ACPI based systems the ID is the ACPI firmware nodes\n> > > + * path. Both ID sources are guaranteed to be unique and persistent as long as\n> > > + * the firmware of the system is not changed.\n> > > + *\n> > > + * A special case is needed to deal with the VIMC pipeline that implements a\n> > > + * virtual pipeline that is not backed by any hardware device and is therefore\n> > > + * not described in the device firmware.\n> > > + *\n> > > + * \\return The sensor ID\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn CameraSensor::entity()\n> > > * \\brief Retrieve the sensor media entity\n> > > @@ -541,4 +568,71 @@ std::string CameraSensor::logPrefix() const\n> > > \treturn \"'\" + entity_->name() + \"'\";\n> > > }\n> > > \n> > > +int CameraSensor::generateID()\n> > > +{\n> > > +\tstd::string path, devPath = subdev_->devicePath();\n> > \n> > We usually split declarations on multiple lines.\n> > \n> > > +\tstruct stat statbuf;\n> > > +\n> > > +\t/* Try to generate ID from OF device tree path. */\n> > > +\tpath = devPath + \"/of_node\";\n> > > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> > \n> > You can use\n> > \n> > \tif (!File::exists(path)) {\n> > \n> > > +\t\tchar ofPath[PATH_MAX];\n> > > +\n> > > +\t\tif (!realpath(path.c_str(), ofPath)) {\n> > \n> > As mentioned in the review of 1/5, should we use realpath with nullptr\n> > as the second argument ? Or maybe just use readlink() with PATH_MAX ?\n> > \n> > > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor OF based ID\";\n> > \n> > s/OF based/OF-based/\n> > \n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tid_ = ofPath;\n> > > +\t\tconst std::string dropStr = \"/sys/firmware/devicetree/\";\n> > \n> > static const ?\n> > \n> > Same comments for the other options below.\n> > \n> > > +\t\tif (id_.find(dropStr) == 0)\n> > > +\t\t\tid_.erase(0, dropStr.length());\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\t/* Try to generate ID from ACPI path. */\n> > > +\tpath = devPath + \"/firmware_node/path\";\n> > > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> > > +\t\tstd::ifstream file(path.c_str());\n> > > +\n> > > +\t\tif (!file.is_open()) {\n> > > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor ACPI based ID\";\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tstd::getline(file, id_);\n> > > +\t\tfile.close();\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * VIMC is a virtual video pipeline not backed hardware and has no OF\n> > > +\t * or ACPI firmware nodes. Handle this pipeline as a special case and\n> > > +\t * generate IDs based on the sensor model.\n> > > +\t */\n> > > +\tpath = devPath + \"/modalias\";\n> > > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> > > +\t\tstd::ifstream file(path.c_str());\n> > > +\n> > > +\t\tif (!file.is_open()) {\n> > > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor ACPI based ID\";\n\nThis shouldn't read \"ACPI\".\n\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tstd::string modalias;\n> > > +\t\tstd::getline(file, modalias);\n> > > +\t\tfile.close();\n> > > +\n> > > +\t\tif (modalias == \"platform:vimc\") {\n> > > +\t\t\tid_ = \"VIMC \" + model();\n> > > +\t\t\treturn 0;\n> > > +\t\t}\n> > \n> > Should we make this case more generic with the following (pseudo-code) ?\n> > \n> > \tid_ = basename(readlink(devPath + \"/device\")) + \"/\" + model_;\n> \n> I don't think so, at least not for now. The only valid use-case to reach \n> this code is for the VIMC pipeline handler (and later perhaps VIVID). I \n> would prefers keeping it strict for now and loosen it as we add \n> pipelines we can test and make sure the id is stable.\n\nWe could keep the driver check, and already use the vimc.0 device name\ninstead of \"VIMC\" to prepare for the future.\n\nAnd given that you will need to factor out the ACPI and OF parts as\nthey're useful for USB too, I wonder if the VIMC special case should be\nkept in CameraSensor, or shouldn't move to the VIMC pipeline handler\ninstead. I'd prefer avoiding pipeline-handler-specific code in\nCameraSensor if possible.\n\n> > > +\t}\n> > > +\n> > > +\tLOG(CameraSensor, Error) << \"Unknown sensor device type, can not generate ID\";\n> > > +\treturn -EINVAL;\n> > > +}\n> > > +\n> > > } /* namespace libcamera */","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 394E0BD86F\n\tfor ;\n\tSun, 2 Aug 2020 21:50:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC713609B2;\n\tSun, 2 Aug 2020 23:50:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 626F260393\n\tfor ;\n\tSun, 2 Aug 2020 23:50:57 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4F5C296;\n\tSun, 2 Aug 2020 23:50:56 +0200 (CEST)"],"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=\"LHxlZcCR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596405057;\n\tbh=kcR/2ZFSonaHfm7gyi1ZxcptAowrGexvA0Pi5E7Dm2g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LHxlZcCRIkPfunJ3R1WkvUyxHNrsgLr1FxZosucXQTZfi815UzDiRel1+obcj10zS\n\tWwrgTQ1Zmtq9zFzWOH9xYbZ8XfU/AyY240oRFK7DCPNIt7CkrzAXrpaj3xw49omX7i\n\t5pqTrIXq3BLUSXR/29iqUR9kXSjKTcgA0k2Eg9z8=","Date":"Mon, 3 Aug 2020 00:50:45 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200802215045.GH23801@pendragon.ideasonboard.com>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-3-niklas.soderlund@ragnatech.se>\n\t<20200802182655.GD23801@pendragon.ideasonboard.com>\n\t<20200802214637.GA1945056@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200802214637.GA1945056@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v5 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]