[{"id":23438,"web_url":"https://patchwork.libcamera.org/comment/23438/","msgid":"","date":"2022-06-17T08:27:04","subject":"Re: [libcamera-devel] [PATCH v5] 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 Fri, 17 Jun 2022 at 07:53, Jacopo Mondi wrote:\n>\n> Hi Eric,\n>\n> On Thu, Jun 16, 2022 at 10:45:52PM +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> > Changes in v5:\n> > - Split into openCard function\n> > - ret initialized to -ENOENT, in case directory is open with no card*\n> > - Add another string to simplify\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 | 62 ++++++++++++++++++++++++++++++++++++-------------\n> > src/cam/drm.h | 2 +-\n> > 2 files changed, 47 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > index 42c5a3b1..39c19aa8 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> > @@ -391,26 +392,55 @@ Device::~Device()\n> > drmClose(fd_);\n> > }\n> >\n> > -int Device::init()\n> > +int Device::openCard()\n> > {\n> > - constexpr size_t NODE_NAME_MAX = sizeof(\"/dev/dri/card255\");\n> > - char name[NODE_NAME_MAX];\n> > - int ret;\n> > + const std::string dirName = \"/dev/dri/\";\n> > + int ret = -ENOENT;\n> >\n> > /*\n> > - * Open the first DRM/KMS device. The libdrm drmOpen*() functions\n> > - * require either a module name or a bus ID, which we don't have, so\n> > - * bypass them. The automatic module loading and device node creation\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> > + * Open the first DRM/KMS device beginning with /dev/dri/card. The\n> > + * libdrm drmOpen*() functions require either a module name or a bus ID,\n> > + * which we don't have, so bypass them. The automatic module loading and\n> > + * device node creation from drmOpen() is of no practical use as any\n> > + * modern system will handle that through udev or an equivalent\n> > + * component.\n> > + */\n>\n> Weird indentation ?\n\nYeah you are right, it appeared on my text editor the same as the old\nindentation. It's spaces instead of tabs here. It's a pity\nclang-format doesn't seem to fix this, nor does checkstyle pick this\nup. Have to do it manually, maybe it's because the tools are ignoring\ncomments indentation, since everything between /* */ is just\nbeautifying a comment.\n\n>\n> > + DIR *folder = opendir(dirName.c_str());\n> > + if (!folder) {\n> > ret = -errno;\n> > - std::cerr\n> > - << \"Failed to open DRM/KMS device \" << name << \": \"\n> > - << strerror(-ret) << std::endl;\n> > + std::cerr << \"Failed to open \" << dirName\n> > + << \" directory: \" << strerror(-ret) << std::endl;\n> > + return ret;\n> > + }\n> > +\n> > + for (struct dirent *res; (res = readdir(folder));) {\n> > + if (strncmp(res->d_name, \"card\", 4)) {\n> > + continue;\n> > + }\n>\n> No need for {}. Does checkstyle warn you ?\n\nNope it doesn't, but I can fix manually.\n\n>\n> > +\n> > + const std::string devName = dirName + res->d_name;\n> > + fd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);\n> > + if (fd_ >= 0) {\n> > + ret = 0;\n> > + break;\n> > + }\n> > +\n> > + ret = -errno;\n> > + std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > + << strerror(-ret) << std::endl;\n> > + }\n> > +\n> > + closedir(folder);\n>\n> Looks like fd_ is still not initialized. Maybe it's not an issue\n> anymore as you now return earlier in init(), but it would have made the\n> code simpler, avoiding you to initialize ret as you could:\n>\n>\n> fd_ = -1;\n> for (res; res = readdir(folder)) {\n> if (res->d_name != \"card)\n> continue;\n>\n> fd_ = open(..);\n> if (fd != -1)\n> break;\n>\n> std::cerr << \"Failed \" ... << strerror(errno);\n> }\n>\n> return fd_ == -1 ? -ENOENT : 0;\n>\n> Up to you\n\nI could initialize fd_ here also, but it would be superfluous if we\nadded here (or become superfluous in the constructor if we added\nwhatever way you want to look at it), I think it's better to have a\nsingle source of truth, if we initialize here, the construct\ninitialization becomes irrelevant, it may as well be -12312. When a\nKMSSink constructor gets called, a Device constructor gets called\n(setting fd_ to -1) and dev_.init() is called in the first line of the\nKMSSink constructor. In this code, you can pretty much treat\nDevice::init() and the constructor as one, they get called in quick\nsuccession, but init() gives you a return code which is handy for\nbailing out early.\n\n>\n> > +\n> > + return ret;\n> > +}\n> > +\n> > +int Device::init()\n> > +{\n> > + int ret = openCard();\n> > + if (ret < 0) {\n> > + std::cerr << \"Failed to open any DRM/KMS device: \"\n> > + << strerror(-ret) << std::endl;\n>\n> Duplicated error message ? The openCard() is already verbose enough\n> maybe ?\n\nThe main reason this error message is here, if there was no message\nhere. in the case you successfully open /dev/dri/, but there is no\nentry in that directory beginning with card*. If we remove this, there\nwould be no error message in this case.\n\n>\n> > return ret;\n> > }\n> >\n> > diff --git a/src/cam/drm.h b/src/cam/drm.h\n> > index de57e445..1817d053 100644\n> > --- a/src/cam/drm.h\n> > +++ b/src/cam/drm.h\n> > @@ -312,7 +312,7 @@ private:\n> > Device &operator=(const Device &&) = delete;\n> >\n> > int getResources();\n> > -\n> > + int openCard();\n> > void drmEvent();\n> > static void pageFlipComplete(int fd, unsigned int sequence,\n> > unsigned int tv_sec, unsigned int tv_usec,\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 71748BD808\n\tfor ;\n\tFri, 17 Jun 2022 08:27:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD75265637;\n\tFri, 17 Jun 2022 10:27:25 +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 BB55D60471\n\tfor ;\n\tFri, 17 Jun 2022 10:27:23 +0200 (CEST)","from mail-qv1-f70.google.com (mail-qv1-f70.google.com\n\t[209.85.219.70]) 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-359-Sdgm59ftPvCCdKsJsg3C9w-1; Fri, 17 Jun 2022 04:27:21 -0400","by mail-qv1-f70.google.com with SMTP id\n\tr14-20020ad4576e000000b0046bbacd783bso4078587qvx.14\n\tfor ;\n\tFri, 17 Jun 2022 01:27:21 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655454445;\n\tbh=1+Krj2j+XgoMyPAiyymHC/XxAAX1VUs3jiMefTASHQo=;\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=Y5OJuqjwt5E4FVgSU484ktFo4GAzYbEQI8DP9qhLc8z4n95pI58zDTwsF9SlooLhD\n\tu48zdGZNeGVPPeEv7uLv4zSiv6vXP74DgCqIKs1lizU5/CfjIUdCwc66D/MAUH7lno\n\tiA5AJUKNQAAuwWCTcawB3iB131fha/sFctsqM6oEZjpjxHDLumDU1cgiEXPBmoxv3l\n\t2N0J65lYUS5hKidNhp+vL1PkiJ4srXHcH51sG0cpF8oV97zfNPoy3FbAHnDACTKcdc\n\t2ykjwA+MxbnyIE0JSOKjKWsgyGjDGX7jcqcgDK8GAtWvlLcrQyubCutfcrtGg5G2VZ\n\t3RLgpJwz9qKwg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1655454442;\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=Lna14sGyJVGkzRfdPrGYUQjOo90v9rhC0QSIdg5Mi8A=;\n\tb=GZNha85/PI37PNntWy5zO3jn9MaAwnARNuS30+RmTMWmv4AJwwKSLRpO4vQ/NWm1DA0iIc\n\t0UcuFTBCAZup+j5x5IDY71mqB1VVlcS+VYqsBgdCW6E0NCmjhmSOioHCUsxECkmxybu4pK\n\twntbA9ZCwPz1Oyk2LeO661kkPMtW2LU="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"GZNha85/\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"Sdgm59ftPvCCdKsJsg3C9w-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=Lna14sGyJVGkzRfdPrGYUQjOo90v9rhC0QSIdg5Mi8A=;\n\tb=pITgahelpoNOY0a6Z3epx41tKL3Lx43y/hlTdvbCXd7PDzoCqDxva4WjMyPzShLsNY\n\t2O/+x8/DcvwOD8QIVTnWbA9cZ36026I4/RxfsFZ4Hp0Muq1ZUw2cOs0hbj8tFDwWVs+D\n\tMEOUrOE3b8XM7WUZg+0aH2KCTRNrIsZfyQLlfi5ZNEeef7oWnbsPRBcZj4tY9PrSUMZq\n\tcrW5TqgBmV52Y5yQAsZuWbtB399iz8m+scH7dRuiXF59qn+L/h9A201rtvxeRZac4KPC\n\tDAzjinIeR+DHH4E/dFQj3s7LIF7if5hQ7ny8E6z8HXGz/yyCX6T6HI6hphS/d1bxsrMg\n\tZV+Q==","X-Gm-Message-State":"AJIora8N+QZW7RRNStV+Q44eRalV64xTDmHpe9YU7MaDFPeFUJia5mrK\n\tCdCJ6GWxUKEYVrE6VG32rVA4C0lGRPxr2WNPBiWGdWL8AtUf/rll6k1LM+1mAWXqiJM8gLY7YfO\n\t+6icddWJUd6Dh976II5dpWcSZXYY305LgWiTLbMBFUpytAdxKlw==","X-Received":["by 2002:a05:6214:e4e:b0:46a:fc75:2719 with SMTP id\n\to14-20020a0562140e4e00b0046afc752719mr7231979qvc.130.1655454441004; \n\tFri, 17 Jun 2022 01:27:21 -0700 (PDT)","by 2002:a05:6214:e4e:b0:46a:fc75:2719 with SMTP id\n\to14-20020a0562140e4e00b0046afc752719mr7231972qvc.130.1655454440756;\n\tFri, 17 Jun 2022 01:27:20 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1tXlben486gB7u226CZMFvpQnYXPnQzLwbWRm0711nkg0nNy3AOk249JKrI+TCo8yluPiA6GVlYF2sdoAFn118=","MIME-Version":"1.0","References":"<20220616214551.43630-1-ecurtin@redhat.com>\n\t<20220617065323.tdqawx55ckj2kn2r@uno.localdomain>","In-Reply-To":"<20220617065323.tdqawx55ckj2kn2r@uno.localdomain>","Date":"Fri, 17 Jun 2022 09:27:04 +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 v5] 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\tJavier Martinez Canillas ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]