[{"id":23282,"web_url":"https://patchwork.libcamera.org/comment/23282/","msgid":"<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>","date":"2022-06-01T17:26:31","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Eric\n\nOn Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> Signed-off-by: Eric Curtin \n> ---\n> src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> 1 file changed, 27 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> index 42c5a3b1..5a322819 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,8 +394,10 @@ 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> +\tconstexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> +\tconstexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> +\tconstexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> +\tchar name[NODE_NAME_MAX] = \"/dev/dri/\";\n> \tint ret;\n>\n> \t/*\n> @@ -404,14 +407,28 @@ 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> -\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\treturn ret;\n> +\tDIR *folder = opendir(name);\n> +\tif (folder) {\n> +\t\tfor (struct dirent *res; (res = readdir(folder));) {\n> +\t\t\tif (strlen(res->d_name) > 4 &&\n\nI feel this might be a bit simplified, maybe using std::filesystem\n\n\tconst std::filesystem::path dri(\"/dev/dri\");\n\tfor (const auto &dir : std::filesystem::directory_iterator(dri)) {\n\t\tconst std::string &direntry = dir.path().filename().string();\n\n\t\tif (direntry.find(\"card\") == std::string::npos)\n\t\t\tcontinue;\n\n\t\tfd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n\n ...\n }\n\n> +\t\t\t !strncmp(res->d_name, \"card\", 4)) {\n> +\t\t\t\tmemcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> +\t\t\t\t BASE_NAME_MAX);\n> +\t\t\t\tfd_ = open(name, O_RDWR | O_CLOEXEC);\n> +\t\t\t\tif (fd_ < 0) {\n> +\t\t\t\t\tret = -errno;\n> +\t\t\t\t\tstd::cerr\n> +\t\t\t\t\t\t<< \"Failed to open DRM/KMS device \"\n> +\t\t\t\t\t\t<< name << \": \"\n> +\t\t\t\t\t\t<< strerror(-ret) << std::endl;\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tclosedir(folder);\n\nWhat if no card is found ?\nShould fd_ be initialized and here checked ?\n\nThanks\n j\n\n> \t}\n>\n> \t/*\n> --\n> 2.35.3\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 588FDBD161\n\tfor ;\n\tWed, 1 Jun 2022 17:26:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A7F660413;\n\tWed, 1 Jun 2022 19:26:35 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B640E60413\n\tfor ;\n\tWed, 1 Jun 2022 19:26:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id F1B8AFF802;\n\tWed, 1 Jun 2022 17:26:32 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654104395;\n\tbh=JnSbojb1MKmX0ojbhNu1FhkQW+aLhsp2taMVzYr8QKU=;\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=TFLec2o8Z/Vb/h05Vyz8kB8aAU91Cr1v6O/C45cLzqzRxd2U6wldHtD7XmtqB5Tz7\n\tK65oKUWI1m3qSjpxe/b5Dw1Gg8A5ak8MbVVqUCFaSlFFfbfM763CfuhRPLZ5Q8o9LY\n\tnsQ7iO1pXYeSBSviIVukNScqPE7dTA3tgm4Ubl6jGrWctm1U4L2drzBe27jvAONfpX\n\tptgUuuKFgE4XTcXY7CLMncNQgdB9TpgImwUugJwuYw3Rv2ZP8ATEPrivplSET1mZFm\n\tG1SnwnNQ/ZhBtlS16jCEH3Iy5u6+5g56J6eeWS/DqaBsukzbbM73z0dPj7CWWBiODe\n\tE8wSYux0PxcHg==","Date":"Wed, 1 Jun 2022 19:26:31 +0200","To":"Eric Curtin ","Message-ID":"<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>","References":"<20220601152345.37753-1-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220601152345.37753-1-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH] 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Ian Mullins , libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23283,"web_url":"https://patchwork.libcamera.org/comment/23283/","msgid":"","date":"2022-06-01T20:41:53","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n>\n> Hi Eric\n>\n> On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > Signed-off-by: Eric Curtin \n> > ---\n> > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > 1 file changed, 27 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> >\n> > int Device::init()\n> > {\n> > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > - char name[NODE_NAME_MAX];\n> > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > int ret;\n> >\n> > /*\n> > @@ -404,14 +407,28 @@ int Device::init()\n> > * from drmOpen() is of no practical use as any modern system will\n> > * handle that through udev or an equivalent component.\n> > */\n> > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > - if (fd_ < 0) {\n> > - ret = -errno;\n> > - std::cerr\n> > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > - << strerror(-ret) << std::endl;\n> > - return ret;\n> > + DIR *folder = opendir(name);\n> > + if (folder) {\n> > + for (struct dirent *res; (res = readdir(folder));) {\n> > + if (strlen(res->d_name) > 4 &&\n>\n> I feel this might be a bit simplified, maybe using std::filesystem\n\nIf ultimately demanded or required, I'll change to std::filesystem, a\nquick grep through the codebase shows that we use opendir and other\nsimilar C code in all instances except for one case in the Android\ncode though. And I would like to keep this code lean and in C if\npossible. In fact in V2 I might make this even leaner and just write\nthe bytes after /dev/dri/card when needed rather than /dev/dri/ and\nremove the strlen.\n\n>\n> const std::filesystem::path dri(\"/dev/dri\");\n> for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> const std::string &direntry = dir.path().filename().string();\n>\n> if (direntry.find(\"card\") == std::string::npos)\n> continue;\n>\n> fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n>\n> ...\n> }\n>\n> > + !strncmp(res->d_name, \"card\", 4)) {\n> > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > + BASE_NAME_MAX);\n> > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > + if (fd_ < 0) {\n> > + ret = -errno;\n> > + std::cerr\n> > + << \"Failed to open DRM/KMS device \"\n> > + << name << \": \"\n> > + << strerror(-ret) << std::endl;\n> > + continue;\n> > + }\n> > +\n> > + break;\n> > + }\n> > + }\n> > +\n> > + closedir(folder);\n>\n> What if no card is found ?\n> Should fd_ be initialized and here checked ?\n\nThanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n\n>\n> Thanks\n> j\n>\n> > }\n> >\n> > /*\n> > --\n> > 2.35.3\n> >\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 386C3BD161\n\tfor ;\n\tWed, 1 Jun 2022 20:42:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F47A65631;\n\tWed, 1 Jun 2022 22:42:14 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 868A260413\n\tfor ;\n\tWed, 1 Jun 2022 22:42:12 +0200 (CEST)","from mail-qt1-f197.google.com (mail-qt1-f197.google.com\n\t[209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-104-0assGjd_NY2n2eeOaMm69g-1; Wed, 01 Jun 2022 16:42:10 -0400","by mail-qt1-f197.google.com with SMTP id\n\tc1-20020ac81101000000b002f9219952f0so2173178qtj.15\n\tfor ;\n\tWed, 01 Jun 2022 13:42:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654116134;\n\tbh=TERJyHndMasR3uJlh89HiNHYCggtoyRQKQtFKjpTHF4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CGww/UP1KyPpR7HiUGfLeaKdYlzFFR4qgz0o1tFMESkvwVHicMZNwaGG6RVXNzQ4M\n\tR0RQOF5wXGsjzKtVLZHlCZFYZYihmmwBYQXt4EQwyW1Qlk6I4B38q/mO+kFxbd7mEp\n\tudZURjy5oFwdfS0cXhxu7EpmrN30m8GGWDB1jeq2mEkuGP4MoJ/bnLSc2TVBWsLNEi\n\t5N2MnOCS8eDN/Zz3ZoLqT7mIpTg6V+8Cf4M331ZRbFf5gi0OmF1c490E/qT7l9Pv0n\n\t7Lti812O8usR4Cgf0tVXjYNEA57HtVipjSz2aOq0ymCATdki19B2T5777r4XnWLFyU\n\txJjt1nQGUI9Sg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1654116131;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=yU/4u/0flVFjOm47xvKelP4dS1eqCSkf30NrAKl0mnc=;\n\tb=fBcKMOiZ0ygjvdyVTMZ5FiJFmJWdOrRK978r2d9kVQ6zu4Uj5EEzIvdB7Es6R0CacxLeJr\n\tc8haTbETNtorO+GBwaZ76opw0pCBgUOatZK/cOD6yovPk1BTpMliYm2aeevJLFp0VSfenm\n\txbvaLjYO8Fz7QjV9ja9uFb5r0sY2prw="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"fBcKMOiZ\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"0assGjd_NY2n2eeOaMm69g-1","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=yU/4u/0flVFjOm47xvKelP4dS1eqCSkf30NrAKl0mnc=;\n\tb=fXGqKomtlLarSYtitJlh0xIqExSpbi2G2S0zWQEtU+jeNdU/PMOEUKxzoYFj5HYEOF\n\tUbeTfOWnR5UoLeua+t2LoO3jtz437rGPDPuoTgIZ9uOqEB9Bh8PZ3IVyv3PLCr92wfW0\n\tM8N1KgH70oL3AclZjET//Ik24GL/t5iULJDCdamj0pHtk9IAhbPu/V/S2IzEoZ8hQmJE\n\tamnaIw8b4OPtafIlKxhG43qciweVPqdloqkuEVALHuWae0UHy1MCU14/PKI00BTsQ+64\n\tBITVFR3Rkbft/iJnrueOFuo6DdnfIdZ4jthj1aH32Fme9O2TqY3MlE3RMhPPJlXWVazR\n\tWZsQ==","X-Gm-Message-State":"AOAM531urXD8dNrDiubR0zkOEQ41dPUANtp6izC6D/gImm4X5l32bj8O\n\tOMklBGHCzwPER9PztfrFrFJ3Brp7rCmk5Su4Uy/Wuy8WhoUt1pvrrtHfe/seBiZaNQ+UoSpcFnf\n\tJERU51WPe/UoOawZV8h9CPruefj1ZzhFerQaTE/Wa+6dSOthi2g==","X-Received":["by 2002:a05:620a:2849:b0:6a6:5998:f743 with SMTP id\n\th9-20020a05620a284900b006a65998f743mr995403qkp.757.1654116129767; \n\tWed, 01 Jun 2022 13:42:09 -0700 (PDT)","by 2002:a05:620a:2849:b0:6a6:5998:f743 with SMTP id\n\th9-20020a05620a284900b006a65998f743mr995393qkp.757.1654116129464;\n\tWed, 01 Jun 2022 13:42:09 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJyhUjVKOdfpapXbl2715OVGF1o7CcR7AKJrspua4C7fueXyW+0ew0EtQq10hZTTrT/lqfNQpFH0XPS+mO1m3EA=","MIME-Version":"1.0","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>","In-Reply-To":"<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>","Date":"Wed, 1 Jun 2022 21:41:53 +0100","Message-ID":"","To":"Jacopo Mondi ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] 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":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"Ian Mullins ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23284,"web_url":"https://patchwork.libcamera.org/comment/23284/","msgid":"<20220602070648.myykiktzktja6bqf@uno.localdomain>","date":"2022-06-02T07:06:48","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Eric,\n\nOn Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote:\n> On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n> >\n> > Hi Eric\n> >\n> > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> > >\n> > > int Device::init()\n> > > {\n> > > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > > - char name[NODE_NAME_MAX];\n> > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > > int ret;\n> > >\n> > > /*\n> > > @@ -404,14 +407,28 @@ int Device::init()\n> > > * from drmOpen() is of no practical use as any modern system will\n> > > * handle that through udev or an equivalent component.\n> > > */\n> > > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > - if (fd_ < 0) {\n> > > - ret = -errno;\n> > > - std::cerr\n> > > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > > - << strerror(-ret) << std::endl;\n> > > - return ret;\n> > > + DIR *folder = opendir(name);\n> > > + if (folder) {\n> > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > + if (strlen(res->d_name) > 4 &&\n> >\n> > I feel this might be a bit simplified, maybe using std::filesystem\n>\n> If ultimately demanded or required, I'll change to std::filesystem, a\n> quick grep through the codebase shows that we use opendir and other\n> similar C code in all instances except for one case in the Android\n> code though. And I would like to keep this code lean and in C if\n> possible. In fact in V2 I might make this even leaner and just write\n> the bytes after /dev/dri/card when needed rather than /dev/dri/ and\n> remove the strlen.\n>\n\nWe have moved rather recently to C++17 for the internal code, where\nstd::filesystem has been introduced. That's maybe why it's not that\nused.\n\nUp to you. However I don't find the previous version much leaner\ncompared to the version I suggested, at least from a readability point\nof view. Why would you like to keep this a much as C code as possible\nif I may ask ?\n\nThanks\n j\n\n> >\n> > const std::filesystem::path dri(\"/dev/dri\");\n> > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > const std::string &direntry = dir.path().filename().string();\n> >\n> > if (direntry.find(\"card\") == std::string::npos)\n> > continue;\n> >\n> > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> >\n> > ...\n> > }\n> >\n> > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > + BASE_NAME_MAX);\n> > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > + if (fd_ < 0) {\n> > > + ret = -errno;\n> > > + std::cerr\n> > > + << \"Failed to open DRM/KMS device \"\n> > > + << name << \": \"\n> > > + << strerror(-ret) << std::endl;\n> > > + continue;\n> > > + }\n> > > +\n> > > + break;\n> > > + }\n> > > + }\n> > > +\n> > > + closedir(folder);\n> >\n> > What if no card is found ?\n> > Should fd_ be initialized and here checked ?\n>\n> Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n>\n> >\n> > Thanks\n> > j\n> >\n> > > }\n> > >\n> > > /*\n> > > --\n> > > 2.35.3\n> > >\n> >\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 A1BCBBD161\n\tfor ;\n\tThu, 2 Jun 2022 07:06:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C937E65631;\n\tThu, 2 Jun 2022 09:06:53 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF2286040E\n\tfor ;\n\tThu, 2 Jun 2022 09:06:51 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 0738CFF810;\n\tThu, 2 Jun 2022 07:06:50 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654153613;\n\tbh=GVeQw4ozM+ptJN7novz2cE9npjCgfcNTbqKx+7w+hbM=;\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=kqpRw84vVRAaE+HcYRMp6FgPDbI1l9WAwoWjVi/GTtxhYEiMk7BLsG9IPiTSuOouC\n\tmQ2N2uXZglTal63c/uWDB0RSu35eVmw9AL7AiaeGQzfFqbZdliRyZQSqiCwiMSY+v9\n\tdPHK/PqObjcoTStDQB1+o1ptT3UW6NT9unWJQTaUngbchp15VfvCOX6QifG49WHApi\n\to2I7mtod08hS6A9Pb2C5Cdo5lWI9mg0DmIDieK3ZuRPuPOMKuyKIQfnXHPCS32R01J\n\tr0I+LBHlHOYAUuwRVPA5kbK+XbRMDGHxgb8zp+DGR/cn9px72MkXNwbBHEfVR6dlel\n\twpv1StGUZ2yQg==","Date":"Thu, 2 Jun 2022 09:06:48 +0200","To":"Eric Curtin ","Message-ID":"<20220602070648.myykiktzktja6bqf@uno.localdomain>","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Ian Mullins ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23288,"web_url":"https://patchwork.libcamera.org/comment/23288/","msgid":"","date":"2022-06-02T08:04:12","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote:\n>\n> Hi Eric,\n>\n> On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote:\n> > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n> > >\n> > > Hi Eric\n> > >\n> > > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> > > >\n> > > > int Device::init()\n> > > > {\n> > > > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > > > - char name[NODE_NAME_MAX];\n> > > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > > > int ret;\n> > > >\n> > > > /*\n> > > > @@ -404,14 +407,28 @@ int Device::init()\n> > > > * from drmOpen() is of no practical use as any modern system will\n> > > > * handle that through udev or an equivalent component.\n> > > > */\n> > > > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > > > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > - if (fd_ < 0) {\n> > > > - ret = -errno;\n> > > > - std::cerr\n> > > > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > > > - << strerror(-ret) << std::endl;\n> > > > - return ret;\n> > > > + DIR *folder = opendir(name);\n> > > > + if (folder) {\n> > > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > > + if (strlen(res->d_name) > 4 &&\n> > >\n> > > I feel this might be a bit simplified, maybe using std::filesystem\n> >\n> > If ultimately demanded or required, I'll change to std::filesystem, a\n> > quick grep through the codebase shows that we use opendir and other\n> > similar C code in all instances except for one case in the Android\n> > code though. And I would like to keep this code lean and in C if\n> > possible. In fact in V2 I might make this even leaner and just write\n> > the bytes after /dev/dri/card when needed rather than /dev/dri/ and\n> > remove the strlen.\n> >\n>\n> We have moved rather recently to C++17 for the internal code, where\n> std::filesystem has been introduced. That's maybe why it's not that\n> used.\n\nI think std::filesystem makes a lot of sense where you have to support\nmany platforms, Linux, Windows, MacOS, etc. and the underlying\nimplementation of the filesystem calls are different, in the\nmulti-platform case I could understand why it doesn't make sense to\nmaintain multiple versions for each platform. But when your only\nplatform is Linux, I feel like it's easier to call the C code directly\nand you get the added benefit of knowing what kind of open calls, etc.\nare used. I feel you don't gain much with the std::filesystem here,\nit's still just a loop with a string comparison. I think it adds a\nlittle complexity here even, like when you see:\n\ndir.path().string().c_str()\n\nand you just need to pass a simple string in there.\n\nOnce you start using a C++ feature you never go back, I always have\nthat little fear also.\n\n>\n> Up to you. However I don't find the previous version much leaner\n> compared to the version I suggested, at least from a readability point\n> of view. Why would you like to keep this a much as C code as possible\n> if I may ask ?\n>\n> Thanks\n> j\n>\n> > >\n> > > const std::filesystem::path dri(\"/dev/dri\");\n> > > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > > const std::string &direntry = dir.path().filename().string();\n> > >\n> > > if (direntry.find(\"card\") == std::string::npos)\n> > > continue;\n> > >\n> > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> > >\n> > > ...\n> > > }\n> > >\n> > > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > > + BASE_NAME_MAX);\n> > > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > + if (fd_ < 0) {\n> > > > + ret = -errno;\n> > > > + std::cerr\n> > > > + << \"Failed to open DRM/KMS device \"\n> > > > + << name << \": \"\n> > > > + << strerror(-ret) << std::endl;\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + closedir(folder);\n> > >\n> > > What if no card is found ?\n> > > Should fd_ be initialized and here checked ?\n> >\n> > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > }\n> > > >\n> > > > /*\n> > > > --\n> > > > 2.35.3\n> > > >\n> > >\n> >\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 12979BD161\n\tfor ;\n\tThu, 2 Jun 2022 08:04:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AA1E65636;\n\tThu, 2 Jun 2022 10:04:33 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9957565633\n\tfor ;\n\tThu, 2 Jun 2022 10:04:30 +0200 (CEST)","from mail-qt1-f199.google.com (mail-qt1-f199.google.com\n\t[209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-602-OXZ1ALaXMvOlyyfYrRFNuw-1; Thu, 02 Jun 2022 04:04:28 -0400","by mail-qt1-f199.google.com with SMTP id\n\tg14-20020ac87d0e000000b00304c6e43d12so2961199qtb.0\n\tfor ;\n\tThu, 02 Jun 2022 01:04:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654157073;\n\tbh=NNAHgLAzRBN1/3EDHQorheSjNhn11KyHiJ0HDOZ9sMM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=SSFw8JPOXP54u61V3PMul7tnI2ci8vnQvGvl9CVFzhnsTeAYGvLxSg2zcweFsTTOS\n\tkrNOO+ZYiKKsGPsUtC2VfKfNdRvVk4Em0tXwn+sLRz7q2A2hqCYPYLtCDuHnXyfN9e\n\tEfGWb57iLzh2brp5T5ZFh3nX8FsX2Gu2h1agd46uiMnSHoV5zfY+lN2IKubPdea2+5\n\tVl91eupKpqW6K65jUiVAjRumLiXZ3159geo/lk0vmyFTZas6eHanxAYR3df3aAAzqh\n\tS/r4YL12WClXPpAHUAKFFnNoEqRxGMX7VTgXfra2LpnvE5T8WH2/qAlBkPgnGVNKmC\n\tArq5XaDVXLCQQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1654157069;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=4e/O2GXRS77mmb/dVFnxOlSZQn90Sp5uRGHTWFtqzBM=;\n\tb=WSvJpIco9YjrJ9gqWFH54sYSrvkUlrLxvi09PZYv8EaciACtjENglTLUD+XjHR7SQSYbBZ\n\tageqv2X7tNpniT76pwq85VVuRcp4C3YM0zW2LMuVoVNMfO4+MxGWk1RybjKMhht6OHXF9g\n\t41KH0wzhTYL3Z1DkIvI0f580bNmHrZ4="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"WSvJpIco\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"OXZ1ALaXMvOlyyfYrRFNuw-1","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=4e/O2GXRS77mmb/dVFnxOlSZQn90Sp5uRGHTWFtqzBM=;\n\tb=sw9z2RbuR0MXb9RfVH+mpCdgM2SEeIzWJle+v1oQIolPgPyczbIYLU/cb/AOmXUwkC\n\t8BZ4DdE50DHj8sAWwrwJbcC64VaBr5uCw4zHCxH5Wfq9Ks4w1nhuLg6i17eQGh/CODtr\n\tA1Ry9fesR4QPi7K/TvxwWDcJV8wOf5eECK9qRuOfAH+Bjsoapsql1TDWaOWSzTUnbRrY\n\tsZE6CV6UTrZuxrPwWkvz8ELcs1m/Q/AewLVUctN7+0FVH5EyYiCksW0vhOu+IAdUcTEO\n\tnC4MvdCbEGcXXYX4zTe4JbxYy/Ov4TcsuDe6F0SCUIcgMJP9LoT6v7JkACGb+/SJggGG\n\tlDIg==","X-Gm-Message-State":"AOAM532+rQGPghxPtpu4xiYoSrp3m7mY7KXmI3mTmsRAuOswmeAX4UL/\n\tOW6HqCA87FnkRhUyse8lbhrQvcgatt3ZPf9JxX76jK/J42BMPVvR8LDKb4+AHPFjFlK2nlEeMaK\n\tLjy5+s4HOhL//kfounE8AfIKpvgFY2fPh8mmcjaDgWNheBh85GQ==","X-Received":["by 2002:a05:620a:2849:b0:6a6:5998:f743 with SMTP id\n\th9-20020a05620a284900b006a65998f743mr2205722qkp.757.1654157068227; \n\tThu, 02 Jun 2022 01:04:28 -0700 (PDT)","by 2002:a05:620a:2849:b0:6a6:5998:f743 with SMTP id\n\th9-20020a05620a284900b006a65998f743mr2205713qkp.757.1654157067962;\n\tThu, 02 Jun 2022 01:04:27 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJxdnI94zu7S5nwABny3iRwVdDkDKbRPycIddYTukJRD95F1yJidsn0SiqacltdQCB/P2e4bny8Li9pf3NG9cBE=","MIME-Version":"1.0","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>\n\t\n\t<20220602070648.myykiktzktja6bqf@uno.localdomain>","In-Reply-To":"<20220602070648.myykiktzktja6bqf@uno.localdomain>","Date":"Thu, 2 Jun 2022 09:04:12 +0100","Message-ID":"","To":"Jacopo Mondi ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] 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":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"Ian Mullins ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23292,"web_url":"https://patchwork.libcamera.org/comment/23292/","msgid":"<20220602084142.ceciaxf76lltb2hg@uno.localdomain>","date":"2022-06-02T08:41:42","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Eric\n\nOn Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote:\n> On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote:\n> >\n> > Hi Eric,\n> >\n> > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote:\n> > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n> > > >\n> > > > Hi Eric\n> > > >\n> > > > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > > > > Signed-off-by: Eric Curtin \n> > > > > ---\n> > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > > > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > > > >\n> > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> > > > >\n> > > > > int Device::init()\n> > > > > {\n> > > > > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > > > > - char name[NODE_NAME_MAX];\n> > > > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > > > > int ret;\n> > > > >\n> > > > > /*\n> > > > > @@ -404,14 +407,28 @@ int Device::init()\n> > > > > * from drmOpen() is of no practical use as any modern system will\n> > > > > * handle that through udev or an equivalent component.\n> > > > > */\n> > > > > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > > > > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > - if (fd_ < 0) {\n> > > > > - ret = -errno;\n> > > > > - std::cerr\n> > > > > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > > > > - << strerror(-ret) << std::endl;\n> > > > > - return ret;\n> > > > > + DIR *folder = opendir(name);\n> > > > > + if (folder) {\n> > > > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > > > + if (strlen(res->d_name) > 4 &&\n> > > >\n> > > > I feel this might be a bit simplified, maybe using std::filesystem\n> > >\n> > > If ultimately demanded or required, I'll change to std::filesystem, a\n> > > quick grep through the codebase shows that we use opendir and other\n> > > similar C code in all instances except for one case in the Android\n> > > code though. And I would like to keep this code lean and in C if\n> > > possible. In fact in V2 I might make this even leaner and just write\n> > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and\n> > > remove the strlen.\n> > >\n> >\n> > We have moved rather recently to C++17 for the internal code, where\n> > std::filesystem has been introduced. That's maybe why it's not that\n> > used.\n>\n> I think std::filesystem makes a lot of sense where you have to support\n> many platforms, Linux, Windows, MacOS, etc. and the underlying\n> implementation of the filesystem calls are different, in the\n> multi-platform case I could understand why it doesn't make sense to\n> maintain multiple versions for each platform. But when your only\n> platform is Linux, I feel like it's easier to call the C code directly\n> and you get the added benefit of knowing what kind of open calls, etc.\n\nWhat is the benefit in knowing that you use opendir + open plus a\nbunch of string comparison API and one memcpy, compared to the C++\nstdlib ?\n\nI see your code doing pretty much canonical things, nothing special\nthat benefits from knowing exactly what lib C functions are used.\n\n> are used. I feel you don't gain much with the std::filesystem here,\n> it's still just a loop with a string comparison. I think it adds a\n> little complexity here even, like when you see:\n>\n> dir.path().string().c_str()\n\nSo, is\n\n+ constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n+ constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n+ constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n+ char name[NODE_NAME_MAX] = \"/dev/dri/\";\n\n+ DIR *folder = opendir(name);\n+ if (folder) {\n+ for (struct dirent *res; (res = readdir(folder));) {\n+ if (strlen(res->d_name) > 4 &&\n+ !strncmp(res->d_name, \"card\", 4)) {\n+ memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n+ BASE_NAME_MAX);\n+\n+ fd_ = open(name, O_RDWR | O_CLOEXEC);\n\neasier to parse than\n\n+ const std::filesystem::path dri(\"/dev/dri\");\n+ for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n+ const std::string &direntry = dir.path().filename().string();\n+\n+ if (direntry.find(\"card\") == std::string::npos)\n+ continue;\n+\n+ fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n\nI guess it's a matter of tastes. I generally don't like C++ syntax, but in\nthis case the benefits are evident. In example reviewing what that memcpy\nexactly does (and let alone the fact you have to go through a copy) took me\na few minutes.\n\n>\n> and you just need to pass a simple string in there.\n>\n> Once you start using a C++ feature you never go back, I always have\n> that little fear also.\n>\n\nWhere do you have to go back to ?\n\nAnyway, it's a little patch and it's not worth a long discussion.\n\nIf you understand C and you prefer it, that's fine. But I might have\nmissed why any of the above arguments is actually relevant.\n\nUp to you ;)\n\n> >\n> > Up to you. However I don't find the previous version much leaner\n> > compared to the version I suggested, at least from a readability point\n> > of view. Why would you like to keep this a much as C code as possible\n> > if I may ask ?\n> >\n> > Thanks\n> > j\n> >\n> > > >\n> > > > const std::filesystem::path dri(\"/dev/dri\");\n> > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > > > const std::string &direntry = dir.path().filename().string();\n> > > >\n> > > > if (direntry.find(\"card\") == std::string::npos)\n> > > > continue;\n> > > >\n> > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> > > >\n> > > > ...\n> > > > }\n> > > >\n> > > > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > > > + BASE_NAME_MAX);\n> > > > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > + if (fd_ < 0) {\n> > > > > + ret = -errno;\n> > > > > + std::cerr\n> > > > > + << \"Failed to open DRM/KMS device \"\n> > > > > + << name << \": \"\n> > > > > + << strerror(-ret) << std::endl;\n> > > > > + continue;\n> > > > > + }\n> > > > > +\n> > > > > + break;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + closedir(folder);\n> > > >\n> > > > What if no card is found ?\n> > > > Should fd_ be initialized and here checked ?\n> > >\n> > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > --\n> > > > > 2.35.3\n> > > > >\n> > > >\n> > >\n> >\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 2EC66BD160\n\tfor ;\n\tThu, 2 Jun 2022 08:41:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FBB665641;\n\tThu, 2 Jun 2022 10:41:46 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61C3665636\n\tfor ;\n\tThu, 2 Jun 2022 10:41:45 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id AB03540009;\n\tThu, 2 Jun 2022 08:41:44 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654159306;\n\tbh=Nowj0JjVI2/0HnFGokdQ1ieydtW1ZRpYRywnTq50bK0=;\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=NmXIzvpdnYdsm+45THX26W83sBumTMFseh2D9FfC5UDMX1OzsLakFgwm6Fmu2VaWv\n\tI/k0NJf5CJz53VASdalkWqGfY+oBCcE2v2lBL5cFPDKe7DWAB0XITIfQ7Kg8NYGRu5\n\t95r+OTqdmheihsXloDS0YqEu6OBOjXcwWdOpxjb1fTPjFr6ejdDzLKG76w74NGe3AE\n\tHgvEOhQNlp0/LcjQCtaXjGlxkxi/MGeWUDbnsIO7ZF9efVMNl+eaYytjrHGOmhZxNC\n\tp+NsaL6J3zOyYTQykmGS25dJIPVsf2sJ0Ny9UmMWLfmXFFVu/4pslOQaIUW7pv7jEw\n\twQcV3rznYY6sQ==","Date":"Thu, 2 Jun 2022 10:41:42 +0200","To":"Eric Curtin ","Message-ID":"<20220602084142.ceciaxf76lltb2hg@uno.localdomain>","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>\n\t\n\t<20220602070648.myykiktzktja6bqf@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Ian Mullins ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23296,"web_url":"https://patchwork.libcamera.org/comment/23296/","msgid":"","date":"2022-06-02T13:50:33","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi wrote:\n>\n> Hi Eric\n>\n> On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote:\n> > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote:\n> > >\n> > > Hi Eric,\n> > >\n> > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote:\n> > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi Eric\n> > > > >\n> > > > > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > > > > > Signed-off-by: Eric Curtin \n> > > > > > ---\n> > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > > > > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> > > > > >\n> > > > > > int Device::init()\n> > > > > > {\n> > > > > > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > > > > > - char name[NODE_NAME_MAX];\n> > > > > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > > > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > > > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > > > > > int ret;\n> > > > > >\n> > > > > > /*\n> > > > > > @@ -404,14 +407,28 @@ int Device::init()\n> > > > > > * from drmOpen() is of no practical use as any modern system will\n> > > > > > * handle that through udev or an equivalent component.\n> > > > > > */\n> > > > > > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > > > > > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > > - if (fd_ < 0) {\n> > > > > > - ret = -errno;\n> > > > > > - std::cerr\n> > > > > > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > > > > > - << strerror(-ret) << std::endl;\n> > > > > > - return ret;\n> > > > > > + DIR *folder = opendir(name);\n> > > > > > + if (folder) {\n> > > > > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > > > > + if (strlen(res->d_name) > 4 &&\n> > > > >\n> > > > > I feel this might be a bit simplified, maybe using std::filesystem\n> > > >\n> > > > If ultimately demanded or required, I'll change to std::filesystem, a\n> > > > quick grep through the codebase shows that we use opendir and other\n> > > > similar C code in all instances except for one case in the Android\n> > > > code though. And I would like to keep this code lean and in C if\n> > > > possible. In fact in V2 I might make this even leaner and just write\n> > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and\n> > > > remove the strlen.\n> > > >\n> > >\n> > > We have moved rather recently to C++17 for the internal code, where\n> > > std::filesystem has been introduced. That's maybe why it's not that\n> > > used.\n> >\n> > I think std::filesystem makes a lot of sense where you have to support\n> > many platforms, Linux, Windows, MacOS, etc. and the underlying\n> > implementation of the filesystem calls are different, in the\n> > multi-platform case I could understand why it doesn't make sense to\n> > maintain multiple versions for each platform. But when your only\n> > platform is Linux, I feel like it's easier to call the C code directly\n> > and you get the added benefit of knowing what kind of open calls, etc.\n>\n> What is the benefit in knowing that you use opendir + open plus a\n> bunch of string comparison API and one memcpy, compared to the C++\n> stdlib ?\n>\n> I see your code doing pretty much canonical things, nothing special\n> that benefits from knowing exactly what lib C functions are used.\n>\n> > are used. I feel you don't gain much with the std::filesystem here,\n> > it's still just a loop with a string comparison. I think it adds a\n> > little complexity here even, like when you see:\n> >\n> > dir.path().string().c_str()\n>\n> So, is\n>\n> + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n>\n> + DIR *folder = opendir(name);\n> + if (folder) {\n> + for (struct dirent *res; (res = readdir(folder));) {\n> + if (strlen(res->d_name) > 4 &&\n> + !strncmp(res->d_name, \"card\", 4)) {\n> + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> + BASE_NAME_MAX);\n> +\n> + fd_ = open(name, O_RDWR | O_CLOEXEC);\n>\n> easier to parse than\n\nTbf, I think the std::filesystem examples are primarily easier to\nparse because of the use of std::string, rather than the use of\nstd::filesystem. std::filesystem results in us replacing:\n\n DIR *folder = opendir(name);\n if (folder) {\n for (struct dirent *res; (res = readdir(folder));) {\n\n...\n\n closedir(folder);\n\nwith:\n\n const std::filesystem::path dri(\"/dev/dri\");\n for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n\nwith std::filesystem, it concerns me to read this:\n\nhttps://en.cppreference.com/w/cpp/filesystem/path/path\n\n\"Exceptions\n\n2, 4-8) May throw implementation-defined exceptions.\"\n\nI'm running this in a scenario where \"/dev/dri\" is often not available\nyet (an early starting camera for automotive), I want to know exactly\nwhat happens in that case (\"man opendir\" clearly states it returns\nnull in that case), I think it will throw an exception, but based on\nthe documentation I don't know for sure. Exception handling is slower\nalso. I also don't know if it's path or directory_iterator that opens\nthe directory, that's important so it can be handled correctly.\n\nThe pre-existing code uses a \"char name[NODE_NAME_MAX];\" over an\nstd::string, I actually think that's ok in this case, because the API\ndefined that as the max size.\n\n>\n> + const std::filesystem::path dri(\"/dev/dri\");\n> + for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> + const std::string &direntry = dir.path().filename().string();\n> +\n> + if (direntry.find(\"card\") == std::string::npos)\n> + continue;\n> +\n> + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n>\n> I guess it's a matter of tastes. I generally don't like C++ syntax, but in\n> this case the benefits are evident. In example reviewing what that memcpy\n> exactly does (and let alone the fact you have to go through a copy) took me\n> a few minutes.\n>\n> >\n> > and you just need to pass a simple string in there.\n> >\n> > Once you start using a C++ feature you never go back, I always have\n> > that little fear also.\n> >\n>\n> Where do you have to go back to ?\n>\n> Anyway, it's a little patch and it's not worth a long discussion.\n>\n> If you understand C and you prefer it, that's fine. But I might have\n> missed why any of the above arguments is actually relevant.\n>\n> Up to you ;)\n>\n> > >\n> > > Up to you. However I don't find the previous version much leaner\n> > > compared to the version I suggested, at least from a readability point\n> > > of view. Why would you like to keep this a much as C code as possible\n> > > if I may ask ?\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > >\n> > > > > const std::filesystem::path dri(\"/dev/dri\");\n> > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > > > > const std::string &direntry = dir.path().filename().string();\n> > > > >\n> > > > > if (direntry.find(\"card\") == std::string::npos)\n> > > > > continue;\n> > > > >\n> > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> > > > >\n> > > > > ...\n> > > > > }\n> > > > >\n> > > > > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > > > > + BASE_NAME_MAX);\n> > > > > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > > + if (fd_ < 0) {\n> > > > > > + ret = -errno;\n> > > > > > + std::cerr\n> > > > > > + << \"Failed to open DRM/KMS device \"\n> > > > > > + << name << \": \"\n> > > > > > + << strerror(-ret) << std::endl;\n> > > > > > + continue;\n> > > > > > + }\n> > > > > > +\n> > > > > > + break;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + closedir(folder);\n> > > > >\n> > > > > What if no card is found ?\n> > > > > Should fd_ be initialized and here checked ?\n> > > >\n> > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > > }\n> > > > > >\n> > > > > > /*\n> > > > > > --\n> > > > > > 2.35.3\n> > > > > >\n> > > > >\n> > > >\n> > >\n> >\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 D7A8CBD161\n\tfor ;\n\tThu, 2 Jun 2022 13:50:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C4F165635;\n\tThu, 2 Jun 2022 15:50:54 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88AE6633A7\n\tfor ;\n\tThu, 2 Jun 2022 15:50:52 +0200 (CEST)","from mail-qk1-f197.google.com (mail-qk1-f197.google.com\n\t[209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-445-ZkNqiqaEPPqDuqRx3mvInw-1; Thu, 02 Jun 2022 09:50:50 -0400","by mail-qk1-f197.google.com with SMTP id\n\tbr42-20020a05620a462a00b006a5b92ed7b2so3695111qkb.19\n\tfor ;\n\tThu, 02 Jun 2022 06:50:50 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654177854;\n\tbh=S5ME6Pa2B5vJGlHQg5yXVuPN/xj0CtBvx/OgA7NfX88=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=K71+l2xa2nKxWgSI0dAtwoPny32gSDqzkZPCOHDBp4ARQp9gEIHq3SrAOXWIbyLx7\n\tRdZNVjPRL02sGy0s5d330UuQsXYiSVZlG+n4hRYBWKpI14T1feRe5ARQ6M8MyH+yDA\n\tIREWRiBQVGyeZWlkYu/U50auHSDNkdjuL4m+aFLOnRU9B3vOF8NfWrynoVvjyN6/pJ\n\tXIiysHFzZDLGnCZtv5gbrTTz6yjGh9qsbgThHrJ5UcE1uhHZBJ5Id4HRJXyXYEpe2q\n\t7Td+MChiXK0dwaAl4vNbcVC2uMfDSbBvKfXitYZmnwfzCXPXF7UO+oVbgrftk5wmVN\n\tOp2JZi9jCiqRw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1654177851;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=mkmyQ5ZxR+qMtzq1dAX6UIDbonQmrkua+S5zIyNv/Ag=;\n\tb=LUFSlpniEee640ELAknhU6hQvfkJ/4KSIRsg/196S/kEMFm1trLoItV32pDDTiW6E/alR7\n\tn8d/8A4gC4G0/cbRWRaq4ZhXUZ+u7j4BquvEZYtnGh8fQCUP1cKk4gIAqMN3iOYpx3vve1\n\tt8rEL/9u0cP+sBCCB9yR+0SB9s8OJkQ="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"LUFSlpni\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"ZkNqiqaEPPqDuqRx3mvInw-1","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=mkmyQ5ZxR+qMtzq1dAX6UIDbonQmrkua+S5zIyNv/Ag=;\n\tb=oAlanqx+wZHijFESnkgr1nH/Gxv0Rxa2aQtB1wii/Yc5gAUKzsbLZlD7qlQAv4AiGC\n\tSgQwfzSwZkO6nsKaoD7iORn3rBs5DQPf81cn1Juo99g+MzhdwZ71VeLIyI1oDrmLAKAG\n\tCgl3CDzQpH0LauwUpIFlmyNZ8aosuKhO/tkcr+3wDLO3KVH7L1zsifroloy6eKnwFnTS\n\tIqYSG7HsLDf9TXtkrmq3EhVvZGwkZNX9eBnFyZMxdPQj3hhgGgWsRedZgAHVVbffNtGp\n\tgzkjNaQE7/VGB7uq4L4lc3CFX2s6pcXqfu9xm391C9+D2hTbJAuUEzOzDFPpqnzHzuBi\n\tAmXA==","X-Gm-Message-State":"AOAM5304y4oqnbIcS2PS2KlJ48msx39YYHxj7Uaphqeo8JHzcoYTpcQp\n\tr4wuM4hfOe8pyVlWjkx3P5024RtesR5yMYrI58saUCSjfBputLww4vICqYTxGEnhi9Qodnhu1Wp\n\tPbxPBbV4dEDOXvxyYGtCUZ4tEOeyBDZkWEKiKZ1eXPf7sq010sA==","X-Received":["by 2002:ac8:5c10:0:b0:303:fd24:eb91 with SMTP id\n\ti16-20020ac85c10000000b00303fd24eb91mr3550693qti.652.1654177849667; \n\tThu, 02 Jun 2022 06:50:49 -0700 (PDT)","by 2002:ac8:5c10:0:b0:303:fd24:eb91 with SMTP id\n\ti16-20020ac85c10000000b00303fd24eb91mr3550684qti.652.1654177849305;\n\tThu, 02 Jun 2022 06:50:49 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJyw0rT/mNlSDncAbQqFRolKUM2e9JIQ6/i+1raQuVMtOxPZOsgVqGlwtgHE5bXYDM885xC1bFk8QVucNO44s8M=","MIME-Version":"1.0","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>\n\t\n\t<20220602070648.myykiktzktja6bqf@uno.localdomain>\n\t\n\t<20220602084142.ceciaxf76lltb2hg@uno.localdomain>","In-Reply-To":"<20220602084142.ceciaxf76lltb2hg@uno.localdomain>","Date":"Thu, 2 Jun 2022 14:50:33 +0100","Message-ID":"","To":"Jacopo Mondi ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] 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":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"Ian Mullins ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23300,"web_url":"https://patchwork.libcamera.org/comment/23300/","msgid":"<20220603063633.wflmwll4hxu2b4jz@uno.localdomain>","date":"2022-06-03T06:36:33","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards\n\tother than 0","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Eric\n\nOn Thu, Jun 02, 2022 at 02:50:33PM +0100, Eric Curtin wrote:\n> On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi wrote:\n> >\n> > Hi Eric\n> >\n> > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote:\n> > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote:\n> > > >\n> > > > Hi Eric,\n> > > >\n> > > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote:\n> > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Hi Eric\n> > > > > >\n> > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > > > > > > Signed-off-by: Eric Curtin \n> > > > > > > ---\n> > > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > > > > > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> > > > > > >\n> > > > > > > int Device::init()\n> > > > > > > {\n> > > > > > > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > > > > > > - char name[NODE_NAME_MAX];\n> > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > > > > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > > > > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > > > > > > int ret;\n> > > > > > >\n> > > > > > > /*\n> > > > > > > @@ -404,14 +407,28 @@ int Device::init()\n> > > > > > > * from drmOpen() is of no practical use as any modern system will\n> > > > > > > * handle that through udev or an equivalent component.\n> > > > > > > */\n> > > > > > > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > > > > > > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > > > - if (fd_ < 0) {\n> > > > > > > - ret = -errno;\n> > > > > > > - std::cerr\n> > > > > > > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > > > > > > - << strerror(-ret) << std::endl;\n> > > > > > > - return ret;\n> > > > > > > + DIR *folder = opendir(name);\n> > > > > > > + if (folder) {\n> > > > > > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > > > > > + if (strlen(res->d_name) > 4 &&\n> > > > > >\n> > > > > > I feel this might be a bit simplified, maybe using std::filesystem\n> > > > >\n> > > > > If ultimately demanded or required, I'll change to std::filesystem, a\n> > > > > quick grep through the codebase shows that we use opendir and other\n> > > > > similar C code in all instances except for one case in the Android\n> > > > > code though. And I would like to keep this code lean and in C if\n> > > > > possible. In fact in V2 I might make this even leaner and just write\n> > > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and\n> > > > > remove the strlen.\n> > > > >\n> > > >\n> > > > We have moved rather recently to C++17 for the internal code, where\n> > > > std::filesystem has been introduced. That's maybe why it's not that\n> > > > used.\n> > >\n> > > I think std::filesystem makes a lot of sense where you have to support\n> > > many platforms, Linux, Windows, MacOS, etc. and the underlying\n> > > implementation of the filesystem calls are different, in the\n> > > multi-platform case I could understand why it doesn't make sense to\n> > > maintain multiple versions for each platform. But when your only\n> > > platform is Linux, I feel like it's easier to call the C code directly\n> > > and you get the added benefit of knowing what kind of open calls, etc.\n> >\n> > What is the benefit in knowing that you use opendir + open plus a\n> > bunch of string comparison API and one memcpy, compared to the C++\n> > stdlib ?\n> >\n> > I see your code doing pretty much canonical things, nothing special\n> > that benefits from knowing exactly what lib C functions are used.\n> >\n> > > are used. I feel you don't gain much with the std::filesystem here,\n> > > it's still just a loop with a string comparison. I think it adds a\n> > > little complexity here even, like when you see:\n> > >\n> > > dir.path().string().c_str()\n> >\n> > So, is\n> >\n> > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> >\n> > + DIR *folder = opendir(name);\n> > + if (folder) {\n> > + for (struct dirent *res; (res = readdir(folder));) {\n> > + if (strlen(res->d_name) > 4 &&\n> > + !strncmp(res->d_name, \"card\", 4)) {\n> > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > + BASE_NAME_MAX);\n> > +\n> > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> >\n> > easier to parse than\n>\n> Tbf, I think the std::filesystem examples are primarily easier to\n> parse because of the use of std::string, rather than the use of\n> std::filesystem. std::filesystem results in us replacing:\n>\n> DIR *folder = opendir(name);\n> if (folder) {\n> for (struct dirent *res; (res = readdir(folder));) {\n>\n> ...\n>\n> closedir(folder);\n>\n> with:\n>\n> const std::filesystem::path dri(\"/dev/dri\");\n> for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n>\n> with std::filesystem, it concerns me to read this:\n>\n> https://en.cppreference.com/w/cpp/filesystem/path/path\n>\n> \"Exceptions\n>\n> 2, 4-8) May throw implementation-defined exceptions.\"\n>\n> I'm running this in a scenario where \"/dev/dri\" is often not available\n> yet (an early starting camera for automotive), I want to know exactly\n> what happens in that case (\"man opendir\" clearly states it returns\n> null in that case), I think it will throw an exception, but based on\n> the documentation I don't know for sure. Exception handling is slower\n> also. I also don't know if it's path or directory_iterator that opens\n> the directory, that's important so it can be handled correctly.\n\nYou're right, it would require an\n\n\tif (!exists(dri))\n\t\treturn -ENOENT;\n\nbefore the for loop\n\n\n>\n> The pre-existing code uses a \"char name[NODE_NAME_MAX];\" over an\n> std::string, I actually think that's ok in this case, because the API\n> defined that as the max size.\n>\n> >\n> > + const std::filesystem::path dri(\"/dev/dri\");\n> > + for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > + const std::string &direntry = dir.path().filename().string();\n> > +\n> > + if (direntry.find(\"card\") == std::string::npos)\n> > + continue;\n> > +\n> > + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> >\n> > I guess it's a matter of tastes. I generally don't like C++ syntax, but in\n> > this case the benefits are evident. In example reviewing what that memcpy\n> > exactly does (and let alone the fact you have to go through a copy) took me\n> > a few minutes.\n> >\n> > >\n> > > and you just need to pass a simple string in there.\n> > >\n> > > Once you start using a C++ feature you never go back, I always have\n> > > that little fear also.\n> > >\n> >\n> > Where do you have to go back to ?\n> >\n> > Anyway, it's a little patch and it's not worth a long discussion.\n> >\n> > If you understand C and you prefer it, that's fine. But I might have\n> > missed why any of the above arguments is actually relevant.\n> >\n> > Up to you ;)\n> >\n> > > >\n> > > > Up to you. However I don't find the previous version much leaner\n> > > > compared to the version I suggested, at least from a readability point\n> > > > of view. Why would you like to keep this a much as C code as possible\n> > > > if I may ask ?\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > >\n> > > > > > const std::filesystem::path dri(\"/dev/dri\");\n> > > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > > > > > const std::string &direntry = dir.path().filename().string();\n> > > > > >\n> > > > > > if (direntry.find(\"card\") == std::string::npos)\n> > > > > > continue;\n> > > > > >\n> > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> > > > > >\n> > > > > > ...\n> > > > > > }\n> > > > > >\n> > > > > > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > > > > > + BASE_NAME_MAX);\n> > > > > > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > > > + if (fd_ < 0) {\n> > > > > > > + ret = -errno;\n> > > > > > > + std::cerr\n> > > > > > > + << \"Failed to open DRM/KMS device \"\n> > > > > > > + << name << \": \"\n> > > > > > > + << strerror(-ret) << std::endl;\n> > > > > > > + continue;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + closedir(folder);\n> > > > > >\n> > > > > > What if no card is found ?\n> > > > > > Should fd_ be initialized and here checked ?\n> > > > >\n> > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n> > > > >\n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > > }\n> > > > > > >\n> > > > > > > /*\n> > > > > > > --\n> > > > > > > 2.35.3\n> > > > > > >\n> > > > > >\n> > > > >\n> > > >\n> > >\n> >\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 E0613BD161\n\tfor ;\n\tFri, 3 Jun 2022 06:36:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E6F665631;\n\tFri, 3 Jun 2022 08:36:40 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9586260104\n\tfor ;\n\tFri, 3 Jun 2022 08:36:38 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2220EFF804;\n\tFri, 3 Jun 2022 06:36:36 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654238200;\n\tbh=RbbsnJeHUDUeniJ96qzms1hnYxxZkHnziAeOeS/OkH4=;\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=gP08YzTXCtjvRdNwHKe8KIw0wrgNbRycXd/r3QJZ0cY8d7Yx3WiX/+vdjNUTa/Y/C\n\tmAc5MovN0YxgyuAiUWszlExgjnDoo/d8WkLXu6YLnELxQMhhk0gfvqWhvKUa9rMNqk\n\te4K3zVEsa/oMalfzROdK9pZ8k2VnSNi3wlPGmVYO3QsqzEAfP+mzldpEsQhJpVJHid\n\tKWLLdfxlL5o9ROAKsewnVbtke0GRMbNyC4PEGowlUuLE6bpVCjvbq883jz8SugP8VF\n\tFI8MuIluMD7SkHHn/LFhgIvB4WrTzCbTVX6ZD2C6Uf8tI0+ALxAx1BqjB0QnfbcyJp\n\tsLVq0npOPYsZg==","Date":"Fri, 3 Jun 2022 08:36:33 +0200","To":"Eric Curtin ","Message-ID":"<20220603063633.wflmwll4hxu2b4jz@uno.localdomain>","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>\n\t\n\t<20220602070648.myykiktzktja6bqf@uno.localdomain>\n\t\n\t<20220602084142.ceciaxf76lltb2hg@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Ian Mullins ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23311,"web_url":"https://patchwork.libcamera.org/comment/23311/","msgid":"","date":"2022-06-03T20:58:45","subject":"Re: [libcamera-devel] [PATCH] 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":"Hello,\n\nOn Fri, Jun 03, 2022 at 08:36:33AM +0200, Jacopo Mondi wrote:\n> On Thu, Jun 02, 2022 at 02:50:33PM +0100, Eric Curtin wrote:\n> > On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi wrote:\n> > > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote:\n> > > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote:\n> > > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:\n> > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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> > > > > > > > Signed-off-by: Eric Curtin \n> > > > > > > > ---\n> > > > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------\n> > > > > > > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > > > > > index 42c5a3b1..5a322819 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,8 +394,10 @@ Device::~Device()\n> > > > > > > >\n> > > > > > > > int Device::init()\n> > > > > > > > {\n> > > > > > > > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > > > > > > > - char name[NODE_NAME_MAX];\n> > > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > > > > > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > > > > > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > > > > > > > int ret;\n> > > > > > > >\n> > > > > > > > /*\n> > > > > > > > @@ -404,14 +407,28 @@ int Device::init()\n> > > > > > > > * from drmOpen() is of no practical use as any modern system will\n> > > > > > > > * handle that through udev or an equivalent component.\n> > > > > > > > */\n> > > > > > > > - snprintf(name, sizeof(name), \"/dev/dri/card%u\", 0);\n> > > > > > > > - fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > > > > - if (fd_ < 0) {\n> > > > > > > > - ret = -errno;\n> > > > > > > > - std::cerr\n> > > > > > > > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > > > > > > > - << strerror(-ret) << std::endl;\n> > > > > > > > - return ret;\n> > > > > > > > + DIR *folder = opendir(name);\n> > > > > > > > + if (folder) {\n> > > > > > > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > > > > > > + if (strlen(res->d_name) > 4 &&\n> > > > > > >\n> > > > > > > I feel this might be a bit simplified, maybe using std::filesystem\n> > > > > >\n> > > > > > If ultimately demanded or required, I'll change to std::filesystem, a\n> > > > > > quick grep through the codebase shows that we use opendir and other\n> > > > > > similar C code in all instances except for one case in the Android\n> > > > > > code though. And I would like to keep this code lean and in C if\n> > > > > > possible. In fact in V2 I might make this even leaner and just write\n> > > > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and\n> > > > > > remove the strlen.\n> > > > >\n> > > > > We have moved rather recently to C++17 for the internal code, where\n> > > > > std::filesystem has been introduced. That's maybe why it's not that\n> > > > > used.\n> > > >\n> > > > I think std::filesystem makes a lot of sense where you have to support\n> > > > many platforms, Linux, Windows, MacOS, etc. and the underlying\n> > > > implementation of the filesystem calls are different, in the\n> > > > multi-platform case I could understand why it doesn't make sense to\n> > > > maintain multiple versions for each platform. But when your only\n> > > > platform is Linux, I feel like it's easier to call the C code directly\n> > > > and you get the added benefit of knowing what kind of open calls, etc.\n> > >\n> > > What is the benefit in knowing that you use opendir + open plus a\n> > > bunch of string comparison API and one memcpy, compared to the C++\n> > > stdlib ?\n> > >\n> > > I see your code doing pretty much canonical things, nothing special\n> > > that benefits from knowing exactly what lib C functions are used.\n> > >\n> > > > are used. I feel you don't gain much with the std::filesystem here,\n> > > > it's still just a loop with a string comparison. I think it adds a\n> > > > little complexity here even, like when you see:\n> > > >\n> > > > dir.path().string().c_str()\n> > >\n> > > So, is\n> > >\n> > > + constexpr size_t DIR_NAME_MAX = sizeof(\"/dev/dri/\");\n> > > + constexpr size_t BASE_NAME_MAX = sizeof(\"card255\");\n> > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;\n> > > + char name[NODE_NAME_MAX] = \"/dev/dri/\";\n> > >\n> > > + DIR *folder = opendir(name);\n> > > + if (folder) {\n> > > + for (struct dirent *res; (res = readdir(folder));) {\n> > > + if (strlen(res->d_name) > 4 &&\n> > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > + BASE_NAME_MAX);\n> > > +\n> > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > >\n> > > easier to parse than\n> >\n> > Tbf, I think the std::filesystem examples are primarily easier to\n> > parse because of the use of std::string, rather than the use of\n> > std::filesystem. std::filesystem results in us replacing:\n> >\n> > DIR *folder = opendir(name);\n> > if (folder) {\n> > for (struct dirent *res; (res = readdir(folder));) {\n> >\n> > ...\n> >\n> > closedir(folder);\n> >\n> > with:\n> >\n> > const std::filesystem::path dri(\"/dev/dri\");\n> > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> >\n> > with std::filesystem, it concerns me to read this:\n> >\n> > https://en.cppreference.com/w/cpp/filesystem/path/path\n> >\n> > \"Exceptions\n> >\n> > 2, 4-8) May throw implementation-defined exceptions.\"\n> >\n> > I'm running this in a scenario where \"/dev/dri\" is often not available\n> > yet (an early starting camera for automotive), I want to know exactly\n> > what happens in that case (\"man opendir\" clearly states it returns\n> > null in that case), I think it will throw an exception, but based on\n> > the documentation I don't know for sure. Exception handling is slower\n> > also. I also don't know if it's path or directory_iterator that opens\n> > the directory, that's important so it can be handled correctly.\n\nThe C++ filesystem API indeed seems a bit underspecified to me here. I\nwould in practice expect the only exceptions to be thrown by\npath::path() to be std::bad_alloc or exceptions related to character\nencoding, which shouldn't be much of a concern. A path object can refer\nto a file or directory that doesn't exist, that won't cause the path\nconstructor to throw an exception.\n\nThe directory_iterator() constructor, however, will thrown an exception\nif the path doesn't exist or is not a directory. A call to\nfs::is_directory() would prevent that.\n\nI think I'm with Jacopo on this, not really due to std::filesystem as\nsuch, but more about usage of std::string. There's nothing\nperformance-sensitive here, so I'd favour the readability and\nmaintainability that std::string provides.\n\nThis being said, I'll review v3.\n\n> You're right, it would require an\n> \n> \tif (!exists(dri))\n> \t\treturn -ENOENT;\n> \n> before the for loop\n> \n> > The pre-existing code uses a \"char name[NODE_NAME_MAX];\" over an\n> > std::string, I actually think that's ok in this case, because the API\n> > defined that as the max size.\n> >\n> > >\n> > > + const std::filesystem::path dri(\"/dev/dri\");\n> > > + for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > > + const std::string &direntry = dir.path().filename().string();\n> > > +\n> > > + if (direntry.find(\"card\") == std::string::npos)\n> > > + continue;\n> > > +\n> > > + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> > >\n> > > I guess it's a matter of tastes. I generally don't like C++ syntax, but in\n> > > this case the benefits are evident. In example reviewing what that memcpy\n> > > exactly does (and let alone the fact you have to go through a copy) took me\n> > > a few minutes.\n> > >\n> > > >\n> > > > and you just need to pass a simple string in there.\n> > > >\n> > > > Once you start using a C++ feature you never go back, I always have\n> > > > that little fear also.\n> > > >\n> > >\n> > > Where do you have to go back to ?\n> > >\n> > > Anyway, it's a little patch and it's not worth a long discussion.\n> > >\n> > > If you understand C and you prefer it, that's fine. But I might have\n> > > missed why any of the above arguments is actually relevant.\n> > >\n> > > Up to you ;)\n> > >\n> > > > > Up to you. However I don't find the previous version much leaner\n> > > > > compared to the version I suggested, at least from a readability point\n> > > > > of view. Why would you like to keep this a much as C code as possible\n> > > > > if I may ask ?\n> > > > >\n> > > > > > >\n> > > > > > > const std::filesystem::path dri(\"/dev/dri\");\n> > > > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) {\n> > > > > > > const std::string &direntry = dir.path().filename().string();\n> > > > > > >\n> > > > > > > if (direntry.find(\"card\") == std::string::npos)\n> > > > > > > continue;\n> > > > > > >\n> > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);\n> > > > > > >\n> > > > > > > ...\n> > > > > > > }\n> > > > > > >\n> > > > > > > > + !strncmp(res->d_name, \"card\", 4)) {\n> > > > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,\n> > > > > > > > + BASE_NAME_MAX);\n> > > > > > > > + fd_ = open(name, O_RDWR | O_CLOEXEC);\n> > > > > > > > + if (fd_ < 0) {\n> > > > > > > > + ret = -errno;\n> > > > > > > > + std::cerr\n> > > > > > > > + << \"Failed to open DRM/KMS device \"\n> > > > > > > > + << name << \": \"\n> > > > > > > > + << strerror(-ret) << std::endl;\n> > > > > > > > + continue;\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + break;\n> > > > > > > > + }\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + closedir(folder);\n> > > > > > >\n> > > > > > > What if no card is found ?\n> > > > > > > Should fd_ be initialized and here checked ?\n> > > > > >\n> > > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!\n> > > > > >\n> > > > > > > > }\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 AEC0DBD161\n\tfor ;\n\tFri, 3 Jun 2022 20:58:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2E2F65633;\n\tFri, 3 Jun 2022 22:58:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 680B860105\n\tfor ;\n\tFri, 3 Jun 2022 22:58:50 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EBBB4A2A;\n\tFri, 3 Jun 2022 22:58:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654289931;\n\tbh=1EXvR9JeRiAjREjs8ZYCzDuRP72Db3z/amdu4cYEsyA=;\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=rFYOUpOSzLp3Em5VVh+rZm/3Kreuiw5sUfFjq9lNIsgK0ZJrMONhPkx6G03zpHhN7\n\t58xlZFY66fBDAeuQuWQ6zhTaG5AmVLGjTsUhWOge7QYLF1nlPYDfRMTQAXkUk8g+yc\n\tgNW2GSZ0gJL23f2doiU6722uj2a0RjQQmWQihg7FoClKuEVcrlrYwwkRUd+WPH/m04\n\tIQoasMubSq0SBbr3BzPGIEQbQYwCZkdud7Ak8517ZR3aFGuEW0I9WPsHkwLBeOKqmQ\n\tQrcGkcw3hiYxyIxNoG4H8PevAdbVM9Kt5VvYvLXofVCl1qDxTgxwGcPnuZjZBu0ASU\n\tkxHcrs/TrplZw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654289930;\n\tbh=1EXvR9JeRiAjREjs8ZYCzDuRP72Db3z/amdu4cYEsyA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CnGEVd80E6j2HLWU7tAT+vuYXB4/B5A0viQvfuHH1opIaU4O9cBZV2iRRXnv3AXYJ\n\t4vhpTblN81DccXAFtDWQIKNDVzkXAfZrhT98w4hKHh1wWpvEfvSjYKYrZsuS7r2oPp\n\tqbigijtrPOnEAWa3eipbEniRnsbpkCVKlPU6dNYM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CnGEVd80\"; dkim-atps=neutral","Date":"Fri, 3 Jun 2022 23:58:45 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220601152345.37753-1-ecurtin@redhat.com>\n\t<20220601172631.jzsvveqlrvvkqa6s@uno.localdomain>\n\t\n\t<20220602070648.myykiktzktja6bqf@uno.localdomain>\n\t\n\t<20220602084142.ceciaxf76lltb2hg@uno.localdomain>\n\t\n\t<20220603063633.wflmwll4hxu2b4jz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220603063633.wflmwll4hxu2b4jz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] 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\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]