[{"id":23362,"web_url":"https://patchwork.libcamera.org/comment/23362/","msgid":"","date":"2022-06-08T22:27:17","subject":"Re: [libcamera-devel] [PATCH v4] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nThank you for the patch.\n\nOn Mon, Jun 06, 2022 at 03:27:45PM +0100, Eric Curtin wrote:\n> Existing code is hardcoded to card0. Since recent fedora upgrades, we\n> have noticed on more than one machine that card1 is present as the\n> lowest numbered device, could theoretically be higher. This technique\n> tries every file starting with card and continue only when we have\n> successfully opened one. These devices with card1 as the lowest device\n> were simply failing when they do not see a /dev/dri/card0 file present.\n> \n> Reported-by: Ian Mullins \n> Tested-by: Ian Mullins \n> Signed-off-by: Eric Curtin \n> \n> ---\n> Changes in v4:\n> - added Tested-by tag\n> - std::stringified things\n> - changed if conditions to reduce indentations\n> - constexpr sizes now don't include null terminator\n> - just return -ENOENT if no device was successfully opened. \n> \n> Changes in v3:\n> - switch back to memcpy, strncpy causing false compiler issue\n> \n> Changes in v2:\n> - return if no valid card found, or directory could not be opened etc.\n> - memcpy to strncpy for safety\n> - only adjust the numerical bytes for each iteration of loop since\n> /dev/dri/card won't change\n> - initialize ret to zero\n> ---\n> src/cam/drm.cpp | 46 +++++++++++++++++++++++++++++++++++++---------\n> 1 file changed, 37 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> index 42c5a3b1..d05b7f5f 100644\n> --- a/src/cam/drm.cpp\n> +++ b/src/cam/drm.cpp\n> @@ -8,6 +8,7 @@\n> #include \"drm.h\"\n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -393,9 +394,15 @@ Device::~Device()\n> \n> int Device::init()\n> {\n> -\tconstexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> -\tchar name[NODE_NAME_MAX];\n> -\tint ret;\n> +\tconstexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\") - 1;\n> +\tconstexpr size_t PRE_NODE_NAME_MAX = sizeof(\"card\") - 1;\n> +\tconstexpr size_t POST_NODE_NAME_MAX = sizeof(\"255\") - 1;\n> +\tconstexpr size_t NODE_NAME_MAX =\n> +\t\tDIR_NAME_MAX + PRE_NODE_NAME_MAX + POST_NODE_NAME_MAX;\n> +\tstd::string name;\n> +\tname.reserve(NODE_NAME_MAX);\n> +\tname = \"/dev/dri/\";\n> +\tint ret = 0;\n\nThis ret need to be initialized to 0 ?\n\n> \n> \t/*\n> \t * Open the first DRM/KMS device. The libdrm drmOpen*() functions\n> @@ -404,16 +411,37 @@ int Device::init()\n> \t * from drmOpen() is of no practical use as any modern system will\n> \t * handle that through udev or an equivalent component.\n> \t */\n> -\tsnprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> -\tfd_ = open(name, O_RDWR | O_CLOEXEC);\n> -\tif (fd_ < 0) {\n> +\tDIR *folder = opendir(name.c_str());\n> +\tif (!folder) {\n> \t\tret = -errno;\n> -\t\tstd::cerr\n> -\t\t\t<< \"Failed to open DRM/KMS device \" << name << \": \"\n> -\t\t\t<< strerror(-ret) << std::endl;\n> +\t\tstd::cerr << \"Failed to open \" << name\n> +\t\t\t << \" directory: \" << strerror(-ret) << std::endl;\n> \t\treturn ret;\n> \t}\n> \n> +\tname += \"card\";\n> +\tfor (struct dirent *res; (res = readdir(folder));) {\n> +\t\tif (!strncmp(res->d_name, \"card\", 4)) {\n> +\t\t\tname += res->d_name + PRE_NODE_NAME_MAX;\n> +\t\t\tfd_ = open(name.c_str(), O_RDWR | O_CLOEXEC);\n> +\t\t\tif (fd_ >= 0) {\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tret = -errno;\n> +\t\t\tstd::cerr << \"Failed to open DRM/KMS device \" << name\n> +\t\t\t\t << \": \" << strerror(-ret) << std::endl;\n> +\t\t\tname.resize(DIR_NAME_MAX + PRE_NODE_NAME_MAX);\n> +\t\t}\n> +\t}\n\nLet's continue simplification of string processing :-)\n\n\tconst std::string dirName{ \"/dev/dri/\" };\n\tDIR *folder = opendir(dirName.c_str());\n\tif (!folder) {\n\t\tret = -errno;\n\t\tstd::cerr << \"Failed to open \" << dirName\n\t\t\t << \" directory: \" << strerror(-ret) << std::endl;\n\t\treturn ret;\n\t}\n\n\tfor (struct dirent *res; (res = readdir(folder)); ) {\n\t\tif (strncmp(res->d_name, \"card\", 4))\n\t\t\tcontinue;\n\n\t\tconst std::string devName = dirName + res->d_name;\n\t\tfd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);\n\t\tif (fd_ >= 0)\n\t\t\tbreak;\n\n\t\tret = -errno;\n\t\tstd::cerr << \"Failed to open DRM/KMS device \" << devName\n\t\t\t << \": \" << strerror(-ret) << std::endl;\n\t}\n\nAnd you can drop the constexpr size_t above. Is it less efficient ?\nProbably, but this is marginal, and not in a hot path.\n\n> +\n> +\tclosedir(folder);\n> +\n> +\tif (fd_ < 0) {\n> +\t\tstd::cerr << \"Failed to open any DRM/KMS device\" << std::endl;\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> \t/*\n> \t * Enable the atomic APIs. This also automatically enables the\n> \t * universal planes API.","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 D5D89BD160\n\tfor ;\n\tWed, 8 Jun 2022 22:27:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30A4865635;\n\tThu, 9 Jun 2022 00:27:25 +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 B8657633A5\n\tfor ;\n\tThu, 9 Jun 2022 00:27:23 +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 E7BD66CF;\n\tThu, 9 Jun 2022 00:27:22 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654727245;\n\tbh=jd5d+mCEFvS14xqeppz7nO4VB9vLG74cirFBb70I5iI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jR29lWf//C+F3ipudcGw3z4yV/nGZstRTia3CZhR1P15Sw+QlYRKNBDffRApOJar1\n\tTZ77Q/LghvHeQJUk4XBZRjcEQ1N++y1jU6/7GsI0u1QHAmvB2zIg943jg+3fpRZeP6\n\tyX7MkKLIRXryLpeFtrrriAcmF3pch9gE440kWYzdHQIMs0d4CZ/dnCyi6ZE4HiMZu5\n\t1HiJf1IrSMX5gxjCFBJ5xh/VB8XryO2h1XfAWo9yBs3aZGINI3IMb9KYpdTlL0KudY\n\tZKn6mMb4hAhshG1FzyFVlo2GmRAEBQ6Jt3xcCiUDOBT6O4pMEFEQGgoPWLMNvMDZDB\n\tanwOT0q+18AZQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654727243;\n\tbh=jd5d+mCEFvS14xqeppz7nO4VB9vLG74cirFBb70I5iI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KpRqkmLj0fydZKDVZUhvKD2dY3G7o6S4J7BUZk+aYquzU611h1CaFme1BqKa6lCKj\n\twWhG8XDUZE3fPKI7AHr/7G6/kR61UO1SRC9Mv1NoruY1824gA78M4P6VaoOORcyyze\n\tuhCbO5tZt93/XMtLbhR5hGGYuhwx4UWZh5t0TaEI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KpRqkmLj\"; dkim-atps=neutral","Date":"Thu, 9 Jun 2022 01:27:17 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220606142743.10104-1-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220606142743.10104-1-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v4] cam: drm: Support /dev/dri cards\n\tother than 0","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Ian Mullins ,\n\tJavier Martinez Canillas ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]