[{"id":11698,"web_url":"https://patchwork.libcamera.org/comment/11698/","msgid":"","date":"2020-07-29T10:23:32","subject":"Re: [libcamera-devel] [PATCH v4 2/5] libcamera: camera_sensor:\n\tGenerate a sensor ID","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 29/07/2020 10:21, 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> ---\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..a12d196ad898bb92 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\nIf my comment about generic catching of virtual hardware is worthy of\nthis series, this can be \"A special case is needed to deal with virtual\npipelines...\" or such.\n\nBut that could also be an 'on top' patch anyway.\n\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> +\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> +\t\tchar ofPath[PATH_MAX];\n> +\n> +\t\tif (!realpath(path.c_str(), ofPath)) {\n> +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor OF based ID\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tid_ = ofPath;\n> +\t\tconst std::string dropStr = \"/sys/firmware/devicetree/\";\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 have no OF\n\ns/have/has/\n\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\n\nHow about instead of making this specific to VIMC, make it the generic\n'last chance/best-effort' fallthrough ?\n\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\nThis could then just be:\n\tid_ = modalias + model()\n\n+ anything else desired to keep things more likely to be unique...\n\n\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n\nI'm being a little selfish of course, but it's because the 'out of tree'\nvivid pipeline handler would not get handled here, and I had\nhoped/intended that it could remain out of tree as an instructional\nseries to support new developers.\n\nThis would 'break' vivid otherwise.\n\nOf course this could potentially also be on top if you don't want to\nsupport it in this series.\n\nLet me know in that case, as I think I'll then have to do it next time I\nrebase the vivid series.\n\ntherefore, with/without making this generic (and the small grammatical\nerror fixed above)\n\nReviewed-by: Kieran Bingham \n\n> +\n> +\tLOG(CameraSensor, Error) << \"Unknown sensor device type, can not generate ID\";\n> +\treturn -EINVAL;\n> +}\n> +\n> } /* namespace libcamera */\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 5384BBD86F\n\tfor ;\n\tWed, 29 Jul 2020 10:23:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0835613C6;\n\tWed, 29 Jul 2020 12:23:36 +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 D3DC56039D\n\tfor ;\n\tWed, 29 Jul 2020 12:23:35 +0200 (CEST)","from [192.168.0.20]\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 4124831F;\n\tWed, 29 Jul 2020 12:23:35 +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=\"L7g2FKkZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596018215;\n\tbh=YCkuG2tD3nfD5TqrHNJ6W2CeduKhVL282qT1RvE9kx8=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=L7g2FKkZgW2MEjF3ds87NB0ky4twgqCkzUeBV3GiY4XVLM3Iq1HD9pwhTHKKwTysT\n\thm+4alO4emsJ6cQ0O1Xx0hasHxbd0t5dXzqPmCqsozUknD8l9ufUoO5o3MFG31YAzr\n\tzYabTuOxPTI6R0dSnh/oSUCvn3y3QLtnUIVy9Lt0=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200729092122.3765539-1-niklas.soderlund@ragnatech.se>\n\t<20200729092122.3765539-3-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 29 Jul 2020 11:23:32 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200729092122.3765539-3-niklas.soderlund@ragnatech.se>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11704,"web_url":"https://patchwork.libcamera.org/comment/11704/","msgid":"<20200729112841.GC322953@oden.dyn.berto.se>","date":"2020-07-29T11:28:41","subject":"Re: [libcamera-devel] [PATCH v4 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 Kieran,\n\nThanks for your comments.\n\nOn 2020-07-29 11:23:32 +0100, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> On 29/07/2020 10:21, 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> > ---\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..a12d196ad898bb92 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> If my comment about generic catching of virtual hardware is worthy of\n> this series, this can be \"A special case is needed to deal with virtual\n> pipelines...\" or such.\n> \n> But that could also be an 'on top' patch anyway.\n> \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> > +\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> > +\t\tchar ofPath[PATH_MAX];\n> > +\n> > +\t\tif (!realpath(path.c_str(), ofPath)) {\n> > +\t\t\tLOG(CameraSensor, Error) << \"Failed to read sensor OF based ID\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tid_ = ofPath;\n> > +\t\tconst std::string dropStr = \"/sys/firmware/devicetree/\";\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 have no OF\n> \n> s/have/has/\n> \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> \n> \n> How about instead of making this specific to VIMC, make it the generic\n> 'last chance/best-effort' fallthrough ?\n\nNo, the idea is that we really should only allow known good devices. So \nI'm afraid a special case for the out-of-tree vivid pipeline is need. I \nthink it would be rather simple tho, see below.\n\n> \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> \n> This could then just be:\n> \tid_ = modalias + model()\n> \n> + anything else desired to keep things more likely to be unique...\n\nI picked this string for VIMC as it is the string used today and we can \nguarantee it's unique. I would not object to chancing this to your \nsuggestion if that is the group suggestion.\n\nI would however not like to have a generic fall-thru here. But adding \nsupport for more virtual hardware should be simple right?\n\n\n else if (modalias == \"platfrom:vivid\")\n id_ = \"VIVID \" ....;\n\n> \n> \n> > +\t\t\treturn 0;\n> > +\t\t}\n> > +\t}\n> \n> I'm being a little selfish of course, but it's because the 'out of tree'\n> vivid pipeline handler would not get handled here, and I had\n> hoped/intended that it could remain out of tree as an instructional\n> series to support new developers.\n\nSelfish is good, we all scratch out itches right :-) I feel your pain of \nmaintaining an out-of-tree pipeline handler, I'm sorry this change \neffects that.\n\n> \n> This would 'break' vivid otherwise.\n> \n> Of course this could potentially also be on top if you don't want to\n> support it in this series.\n> \n> Let me know in that case, as I think I'll then have to do it next time I\n> rebase the vivid series.\n\nI don't intend to make this a generic fall-true unless I'm seriously \npushed in that direction from the group :-)\n\n> \n> therefore, with/without making this generic (and the small grammatical\n> error fixed above)\n> \n> Reviewed-by: Kieran Bingham \n\nThanks!\n\n> \n> > +\n> > +\tLOG(CameraSensor, Error) << \"Unknown sensor device type, can not generate ID\";\n> > +\treturn -EINVAL;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","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 3FFBABD878\n\tfor ;\n\tWed, 29 Jul 2020 11:28:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D018F613C6;\n\tWed, 29 Jul 2020 13:28:44 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FE936039D\n\tfor ;\n\tWed, 29 Jul 2020 13:28:43 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id g6so12009625ljn.11\n\tfor ;\n\tWed, 29 Jul 2020 04:28:43 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tr22sm341939ljc.25.2020.07.29.04.28.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 29 Jul 2020 04:28:41 -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=\"HO8fbV2Z\"; 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=o/QBfbu3zjPbWdApm/7hXqmder6jZzpsg3R0GxdSuRs=;\n\tb=HO8fbV2Zs6FwLEkjzjPGiCgQS9X41EnhWLzI7FGACQ4oedHir2/SOTgRAmGHxRW3a6\n\t56teOuQ0agr0mFpz1viSXmpKjtY9eOpYDpfGGbUTn03kWC1Pz1CfX0qDeNjxjjPQ8/TX\n\tfPXlO9iKwxfBVQjLQTAQ/V6Av8hRGH4T5e5lB83vhtGWIqM2y7THbrS/S+KLykU/h/TA\n\twoRVdrT0wS2MhPUbEVvgUVgxllElqG07LWZ2AsmaEz/R9yUyBbMD5XRMniwCFgQXhIP8\n\tfHmr0TbstjmO/Bejw6kKHdJXjtMwfB04kuy9lIgKSLhYjUPhUawzgo9VqbRIzZNvFkGI\n\tWSxw==","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=o/QBfbu3zjPbWdApm/7hXqmder6jZzpsg3R0GxdSuRs=;\n\tb=cKJbeXNKRbKSDbnXZc/Tca5L2yOZUJFGgLgq/3QGmU0MQJlPvFcXZPFPNwXFG7kFLz\n\tRbqQUx9yu5b+IRkixCPz/HU5KO5hqhdk5sbZOstvsSRuDAYN/o3+wnot5+aMz+dyCmMo\n\tNNEcDmIxhx56l4oyqT5NW8RyO63U6in4F/y/fn6batNX9pjPhygobBeVxDtQO85u9Es7\n\tS4DcJBTPfvXr9vSZG7gsRcUsHbIzzcqt921tnEcA7H7vwRZH/DnCypWNyYmWWHOOVESR\n\tB6u7ouDmAKhZLFBJDy+1yhLRvQDLjskX9Aebk0hN+p3y5f65ktlYUS6LwxoDjpc5dt9a\n\te9PQ==","X-Gm-Message-State":"AOAM530LiqeAozMYg7+bQKt8rq5NtSvpA0RHTxRpqFfCZzfEAgB+P7ym\n\tbWVo0HSmSjzXrIlC28rFHxO0MZWC538=","X-Google-Smtp-Source":"ABdhPJyg2oX2IJTk/YRaTrBevcSofTZeCNaRiUQA0O9VbuHduGv99NWHD+ulZSRnIoDCh5m1emKeHA==","X-Received":"by 2002:a05:651c:3cc:: with SMTP id\n\tf12mr2257266ljp.335.1596022122504; \n\tWed, 29 Jul 2020 04:28:42 -0700 (PDT)","Date":"Wed, 29 Jul 2020 13:28:41 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Message-ID":"<20200729112841.GC322953@oden.dyn.berto.se>","References":"<20200729092122.3765539-1-niklas.soderlund@ragnatech.se>\n\t<20200729092122.3765539-3-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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\" "}}]