[{"id":25141,"web_url":"https://patchwork.libcamera.org/comment/25141/","msgid":"<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>","date":"2022-09-28T06:10:02","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The DRM helper picks the first DRM card that it can open. On platforms\n> that have a standalone GPU, this risks selecting a device corresponding\n> to the GPU instead of the display controller. Fix this by skipping\n> devices that don't support the KMS atomic API. Some legacy display\n> controllers would be skipped as well, but libcamera doesn't run on those\n> systems anyway, and the DRM helper doesn't support the legacy KMS\n> modeset API in any case.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/cam/drm.cpp | 31 +++++++++++++++++++++++--------\n> 1 file changed, 23 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> index b0602c942853..b2a6b0bba9e8 100644\n> --- a/src/cam/drm.cpp\n> +++ b/src/cam/drm.cpp\n> @@ -430,7 +430,8 @@ int Device::init()\n> int Device::openCard()\n> {\n> \tconst std::string dirName = \"/dev/dri/\";\n> -\tint ret = -ENOENT;\n> +\tbool found = false;\n> +\tint ret;\n>\n> \t/*\n> \t * Open the first DRM/KMS device beginning with /dev/dri/card. The\n> @@ -449,24 +450,38 @@ int Device::openCard()\n> \t}\n>\n> \tfor (struct dirent *res; (res = readdir(folder));) {\n> +\t\tuint64_t cap;\n> +\n> \t\tif (strncmp(res->d_name, \"card\", 4))\n> \t\t\tcontinue;\n>\n> \t\tconst std::string devName = dirName + res->d_name;\n> \t\tfd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);\n> -\t\tif (fd_ >= 0) {\n> -\t\t\tret = 0;\n> -\t\t\tbreak;\n> +\t\tif (fd_ < 0) {\n> +\t\t\tret = -errno;\n\nAs we don't propagate errors up, can't you use errno directly ?\n\n> +\t\t\tstd::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> +\t\t\t\t << strerror(-ret) << std::endl;\n> +\t\t\tcontinue;\n> \t\t}\n>\n> -\t\tret = -errno;\n> -\t\tstd::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> -\t\t\t << strerror(-ret) << std::endl;\n> +\t\t/*\n> +\t\t * Skip devices that don't support the atomic API, to avoid\n> +\t\t * selecting a DRM device corresponding to a GPU.\n> +\t\t */\n> +\t\tret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);\n\nThis is the only reference I found of drmGetCap:\nhttps://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/\n\nCan it return a negative value ?\n\n\n> +\t\tif (ret < 0) {\n> +\t\t\tdrmClose(fd_);\n> +\t\t\tfd_ = -1;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tfound = true;\n> +\t\tbreak;\n> \t}\n>\n> \tclosedir(folder);\n>\n> -\treturn ret;\n> +\treturn found ? 0 : -ENOENT;\n> }\n>\n> int Device::getResources()\n> --\n> Regards,\n>\n> Laurent Pinchart\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 6F6F8BD16B\n\tfor ;\n\tWed, 28 Sep 2022 06:10:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B559762280;\n\tWed, 28 Sep 2022 08:10:04 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3517961F78\n\tfor ;\n\tWed, 28 Sep 2022 08:10:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A5819FF80C;\n\tWed, 28 Sep 2022 06:10:03 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664345404;\n\tbh=OgyWjOVjiS8JmhJfJoajnmgIEEvlDROy8YlL1O0W/p8=;\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=QpRT4z4asD1Xa3hZlzVbP86RZU3Zh0uhsQCyKZsrXex5Or88oD/WQg5MQOdSXMmyN\n\tfk+9UVC6pR3TG/+pUDv/3IIU3cBJgK42k2HjIhvX9l+wSuaF5Vf8mtfBNfxNdw0e07\n\tO/LRK9uqBGnXkXiOB8CWkKY49FCp+Z4kf5JnqF6uOxo5ROEFMS9mbaChpTnLLtM0WI\n\tj5o3cTkdUOhLp/i6CkNdSzJuWiMxDNaMXzD5r+uE+64ZHXJOaXidrG1qDN82870HiV\n\thQWOC9xbmjBgQAseoyXmqz+MAwzzfysqF3pRQZEm4SSwwqJbJf6a4tPkpNWPxy87FN\n\tSto7ie0yLCQnw==","Date":"Wed, 28 Sep 2022 08:10:02 +0200","To":"Laurent Pinchart ","Message-ID":"<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>","References":"<20220927232628.7544-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220927232628.7544-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25155,"web_url":"https://patchwork.libcamera.org/comment/25155/","msgid":"","date":"2022-09-28T08:52:14","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 28, 2022 at 08:10:02AM +0200, Jacopo Mondi wrote:\n> On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > The DRM helper picks the first DRM card that it can open. On platforms\n> > that have a standalone GPU, this risks selecting a device corresponding\n> > to the GPU instead of the display controller. Fix this by skipping\n> > devices that don't support the KMS atomic API. Some legacy display\n> > controllers would be skipped as well, but libcamera doesn't run on those\n> > systems anyway, and the DRM helper doesn't support the legacy KMS\n> > modeset API in any case.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/cam/drm.cpp | 31 +++++++++++++++++++++++--------\n> > 1 file changed, 23 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > index b0602c942853..b2a6b0bba9e8 100644\n> > --- a/src/cam/drm.cpp\n> > +++ b/src/cam/drm.cpp\n> > @@ -430,7 +430,8 @@ int Device::init()\n> > int Device::openCard()\n> > {\n> > \tconst std::string dirName = \"/dev/dri/\";\n> > -\tint ret = -ENOENT;\n> > +\tbool found = false;\n> > +\tint ret;\n> >\n> > \t/*\n> > \t * Open the first DRM/KMS device beginning with /dev/dri/card. The\n> > @@ -449,24 +450,38 @@ int Device::openCard()\n> > \t}\n> >\n> > \tfor (struct dirent *res; (res = readdir(folder));) {\n> > +\t\tuint64_t cap;\n> > +\n> > \t\tif (strncmp(res->d_name, \"card\", 4))\n> > \t\t\tcontinue;\n> >\n> > \t\tconst std::string devName = dirName + res->d_name;\n> > \t\tfd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);\n> > -\t\tif (fd_ >= 0) {\n> > -\t\t\tret = 0;\n> > -\t\t\tbreak;\n> > +\t\tif (fd_ < 0) {\n> > +\t\t\tret = -errno;\n> \n> As we don't propagate errors up, can't you use errno directly ?\n\nThe first calls to operator<<() may modify the value of errno before the\nstrerror() call is evaluated. See\nhttps://en.cppreference.com/w/cpp/language/eval_order:\n\n\"Order of evaluation of any part of any expression, including order of\nevaluation of function arguments is unspecified (with some exceptions\nlisted below). The compiler can evaluate operands and other\nsubexpressions in any order, and may choose another order when the same\nexpression is evaluated again.\"\n\n> > +\t\t\tstd::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > +\t\t\t\t << strerror(-ret) << std::endl;\n> > +\t\t\tcontinue;\n> > \t\t}\n> >\n> > -\t\tret = -errno;\n> > -\t\tstd::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > -\t\t\t << strerror(-ret) << std::endl;\n> > +\t\t/*\n> > +\t\t * Skip devices that don't support the atomic API, to avoid\n> > +\t\t * selecting a DRM device corresponding to a GPU.\n> > +\t\t */\n> > +\t\tret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);\n> \n> This is the only reference I found of drmGetCap:\n> https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/\n> \n> Can it return a negative value ?\n\nhttps://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L1383 for\nthe implementation of the function, which just calls DRM_IOCTL_GET_CAP.\nThat ends up in drm_getcap() on the kernel side, which, for\nDRM_CLIENT_CAP_ATOMIC, ... works by chance, as DRM_IOCTL_GET_CAP take a\nDRM_CAP_* argument, not a DRM_CLIENT_CAP_* argument.\nDRM_CLIENT_CAP_ATOMIC is equal to 3, which is\nDRM_CAP_DUMB_PREFERRED_DEPTH. I'll fix this to use DRM_CAP_DUMB_BUFFER.\nIt doesn't matter much what cap we read, the important part is to pick\none that returns an error for non-KMS drivers.\n\n> > +\t\tif (ret < 0) {\n> > +\t\t\tdrmClose(fd_);\n> > +\t\t\tfd_ = -1;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tfound = true;\n> > +\t\tbreak;\n> > \t}\n> >\n> > \tclosedir(folder);\n> >\n> > -\treturn ret;\n> > +\treturn found ? 0 : -ENOENT;\n> > }\n> >\n> > int Device::getResources()","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 D5244C0DA4\n\tfor ;\n\tWed, 28 Sep 2022 08:52:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3306A6229F;\n\tWed, 28 Sep 2022 10:52:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A201461F7A\n\tfor ;\n\tWed, 28 Sep 2022 10:52:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DFCA147C;\n\tWed, 28 Sep 2022 10:52:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664355137;\n\tbh=s7GTt7+QFJJSGbITaqCgSSMajdwx5Z/ba+hB1FsVa8w=;\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=Nn4xarTPnNjgGmXJ3gzoqNURr0t3Xo90T9BdyW02AWigZqI3pvcFzHTrELu64iTdR\n\tgEn+GmuAgZbb/ZXV5rirgM3oRjBQgFPCULaGGG1/ljWojk4WSPOdO9FNp4eKf9fUlV\n\tAvzzgNNhDinGMk6hdYMAtAiuSMzwqxpiq0ri5zoiL4KErrG6rOY6nkY6KqJ/gSo9PU\n\tt75uTc9LuOduISQH93VqM2/PXmXl1LbTicc3BbtknQ0iOnC3rK9GHe5ApQd3f5wd0L\n\tjmrCzK1uZ9mjcFpZmFMXvIdUM+KUpw9ETQdZ6CL+fdL/qYIoBDn21pXWWWHVzWnVSm\n\tI16l8m5dLkNkg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664355135;\n\tbh=s7GTt7+QFJJSGbITaqCgSSMajdwx5Z/ba+hB1FsVa8w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WiXbQTSXbQ3YsE5JauU6Cn2bLCefKuvbCtgDQu5fhWstNz945LtNaFCJRmLBjg4dK\n\tAOGwTgp+gC3kiRPANrE5cm9GOvL1bdbiIq2HSUmMyTFNbnl/WYEucAsaGdWh0s82WB\n\tvkmjhuc2hfno3OKe27HYuG/ylBC7v95KzmTMJmgE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WiXbQTSX\"; dkim-atps=neutral","Date":"Wed, 28 Sep 2022 11:52:14 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220927232628.7544-1-laurent.pinchart@ideasonboard.com>\n\t<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25156,"web_url":"https://patchwork.libcamera.org/comment/25156/","msgid":"","date":"2022-09-28T09:57:29","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Wed, 28 Sept 2022 at 09:52, Laurent Pinchart via libcamera-devel\n wrote:\n>\n> Hi Jacopo,\n>\n> On Wed, Sep 28, 2022 at 08:10:02AM +0200, Jacopo Mondi wrote:\n> > On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > The DRM helper picks the first DRM card that it can open. On platforms\n> > > that have a standalone GPU, this risks selecting a device corresponding\n> > > to the GPU instead of the display controller. Fix this by skipping\n> > > devices that don't support the KMS atomic API. Some legacy display\n> > > controllers would be skipped as well, but libcamera doesn't run on those\n> > > systems anyway, and the DRM helper doesn't support the legacy KMS\n> > > modeset API in any case.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/cam/drm.cpp | 31 +++++++++++++++++++++++--------\n> > > 1 file changed, 23 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > index b0602c942853..b2a6b0bba9e8 100644\n> > > --- a/src/cam/drm.cpp\n> > > +++ b/src/cam/drm.cpp\n> > > @@ -430,7 +430,8 @@ int Device::init()\n> > > int Device::openCard()\n> > > {\n> > > const std::string dirName = \"/dev/dri/\";\n> > > - int ret = -ENOENT;\n> > > + bool found = false;\n> > > + int ret;\n> > >\n> > > /*\n> > > * Open the first DRM/KMS device beginning with /dev/dri/card. The\n> > > @@ -449,24 +450,38 @@ int Device::openCard()\n> > > }\n> > >\n> > > for (struct dirent *res; (res = readdir(folder));) {\n> > > + uint64_t cap;\n> > > +\n> > > if (strncmp(res->d_name, \"card\", 4))\n> > > continue;\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> > > + if (fd_ < 0) {\n> > > + ret = -errno;\n> >\n> > As we don't propagate errors up, can't you use errno directly ?\n>\n> The first calls to operator<<() may modify the value of errno before the\n> strerror() call is evaluated. See\n> https://en.cppreference.com/w/cpp/language/eval_order:\n>\n> \"Order of evaluation of any part of any expression, including order of\n> evaluation of function arguments is unspecified (with some exceptions\n> listed below). The compiler can evaluate operands and other\n> subexpressions in any order, and may choose another order when the same\n> expression is evaluated again.\"\n\nYeah, I think what Laurent did with the extra variable is safer, just\nin case errno changes.\n\n>\n> > > + std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > > + << strerror(-ret) << std::endl;\n> > > + continue;\n> > > }\n> > >\n> > > - ret = -errno;\n> > > - std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > > - << strerror(-ret) << std::endl;\n> > > + /*\n> > > + * Skip devices that don't support the atomic API, to avoid\n> > > + * selecting a DRM device corresponding to a GPU.\n> > > + */\n> > > + ret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);\n> >\n> > This is the only reference I found of drmGetCap:\n> > https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/\n> >\n> > Can it return a negative value ?\n\nIt's important to read the source code on gitlab for drm, because they\naccept gitlab Merge Requests as well as submissions via mailing list\n(like Laurent alluded to). My last patch for drm was accepted via\ngitlab, so it probably doesn't appear on the mailing list as an\nexample. So looking at:\n\nhttps://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c\n\nI think what's most correct is `if (ret || !cap)` from looking at the\noutermost drmGetCap function, the cap.value only gets set if we return\n0 (and as a secondary check, make sure capability is set to non-zero).\nPersonally I think this is better as the reader can identify what is\nintended without going into kernel-space code. I also saw this logic\nin Chromium OS (well they do the inverse):\n\nhttps://chromium.googlesource.com/chromiumos/platform/frecon/+/refs/heads/main/drm.c\n\nwith this change:\n\nReviewed-by: Eric Curtin \n\n>\n> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L1383 for\n> the implementation of the function, which just calls DRM_IOCTL_GET_CAP.\n> That ends up in drm_getcap() on the kernel side, which, for\n> DRM_CLIENT_CAP_ATOMIC, ... works by chance, as DRM_IOCTL_GET_CAP take a\n> DRM_CAP_* argument, not a DRM_CLIENT_CAP_* argument.\n> DRM_CLIENT_CAP_ATOMIC is equal to 3, which is\n> DRM_CAP_DUMB_PREFERRED_DEPTH. I'll fix this to use DRM_CAP_DUMB_BUFFER.\n> It doesn't matter much what cap we read, the important part is to pick\n> one that returns an error for non-KMS drivers.\n>\n> > > + if (ret < 0) {\n> > > + drmClose(fd_);\n> > > + fd_ = -1;\n> > > + continue;\n> > > + }\n> > > +\n> > > + found = true;\n> > > + break;\n> > > }\n> > >\n> > > closedir(folder);\n> > >\n> > > - return ret;\n> > > + return found ? 0 : -ENOENT;\n> > > }\n> > >\n> > > int Device::getResources()\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 0F990BD16B\n\tfor ;\n\tWed, 28 Sep 2022 09:57:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95EDC622A0;\n\tWed, 28 Sep 2022 11:57:51 +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 B212861F7A\n\tfor ;\n\tWed, 28 Sep 2022 11:57:49 +0200 (CEST)","from mail-vs1-f71.google.com (mail-vs1-f71.google.com\n\t[209.85.217.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id\n\tus-mta-495-3snBRODrPn-sNK8qeldpiA-1; Wed, 28 Sep 2022 05:57:47 -0400","by mail-vs1-f71.google.com with SMTP id\n\ta6-20020a671a06000000b003986a4e277dso2099683vsa.12\n\tfor ;\n\tWed, 28 Sep 2022 02:57:47 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664359071;\n\tbh=OsD1Hw+74VwBlbAgx/yCMBrh7tX9jPl8tcbrZywtfgA=;\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=IqyxeQ2YS1xmfu+HVe4rkAQ2JHFeSFekl8mHil66DrVFO2+s3EXR0vuL1ipbhCaBm\n\t0NNeZSXjxa98Gor3/rQGEImLF6iqhvy5cizw7C4Z3qAd2U5RqNjkLaa6e9Bgq2xP/1\n\tRTg8XTf7Cnh+ZOHNn6YGZ19+3T7dhaL+gW5e5C9Ue75r1/U4+yMCreWAUg8uWD3m7O\n\tYl+cwcn1U37oNTjhp4QROA8NlafuqlHKXREC9N5hF2FDcZVWvnwSt3GVApKVTDxj3y\n\t7gA7CduMdMrjqzbaEUVtyJZmw06MPswCf6Z/Rf6weX5RMhuIzv8l9rz2tTTfAEhESN\n\tvGWEXIOkSrVKQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1664359068;\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=/j+CYIIe/iT33WXLBag+AAwszA6PJqcvxqCNOaf1lhI=;\n\tb=A9/fd7mSAPkXy5YDyF0Luchztcd726H958g34nCoG/7QA2KvSxFABCMGZt/7D5TDTYwIhh\n\tFS8ufkYsPVzYMrJHn9ANNgIWF2+TWAhlPIoft/kU9YVjLNBR3wpyQroI6h4tq3UhWPWaez\n\tEI6QCBGc+71MysgehiSscTrquWlvo6k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"A9/fd7mS\"; \n\tdkim-atps=neutral","X-MC-Unique":"3snBRODrPn-sNK8qeldpiA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=/j+CYIIe/iT33WXLBag+AAwszA6PJqcvxqCNOaf1lhI=;\n\tb=NdRpj29Nq0gPTt/Thp8ONNirwIHvrmxAjcl1i974Ow7txddNgyiax3HR7cAsGBcaI2\n\tHJXmV1RxIlhQQ9FB+7Q8ek7ZEcf0jIAyVFKimXdgff3vAB6fDWMCsnCnFTOTemtSPqbE\n\tSmQFV8DDukRCXiEBQw6n6Tvil2aqXqyZlZCwYySgykJNkpU7I3oPWmbLz7drv2L7njFK\n\tFscgql6j7zxR7dOVpSuYfaXr1dbhHjyL0BacoUlBBcDfb756SxD3PqFxG2+0MrUdXOhD\n\tku/1D3Qew6W0GBfqbSf4Max2IW2E9QZ77lHvxvTD63/vfuikC1biNRX2af/lTNPaVwW3\n\t2gzw==","X-Gm-Message-State":"ACrzQf1UWX/vCFvFq/MU0ISXEQoiBstw7zRUZwR4EFsEX/yhxqRULA+F\n\tOriclND3NDcpte1xxbJ5vJSCUiHSEyJKgVyhajxSvzhMdBjJSBzjA+jG0xp4OOhHtA28u1TwmZa\n\tQ2DqnV+jLFwjEIMItWeiZtZPSnpQAC+r3cpl6GPE5IZf3LTeZfg==","X-Received":["by 2002:ab0:7e4e:0:b0:39f:84e0:32f3 with SMTP id\n\te14-20020ab07e4e000000b0039f84e032f3mr14505008uax.106.1664359065366; \n\tWed, 28 Sep 2022 02:57:45 -0700 (PDT)","by 2002:ab0:7e4e:0:b0:39f:84e0:32f3 with SMTP id\n\te14-20020ab07e4e000000b0039f84e032f3mr14504996uax.106.1664359065130;\n\tWed, 28 Sep 2022 02:57:45 -0700 (PDT)"],"X-Google-Smtp-Source":"AMsMyM4X+A9J0Yl4JN2uPrLpdr/ZWFsnHW6FdRlibWvdCXVqV9goy3h/p6arYvwqE0yH8hNTAouHukux4v5RF1zsCa8=","MIME-Version":"1.0","References":"<20220927232628.7544-1-laurent.pinchart@ideasonboard.com>\n\t<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>\n\t","In-Reply-To":"","Date":"Wed, 28 Sep 2022 10:57:29 +0100","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25158,"web_url":"https://patchwork.libcamera.org/comment/25158/","msgid":"","date":"2022-09-28T10:09:39","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Wed, Sep 28, 2022 at 10:57:29AM +0100, Eric Curtin wrote:\n> On Wed, 28 Sept 2022 at 09:52, Laurent Pinchart wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Wed, Sep 28, 2022 at 08:10:02AM +0200, Jacopo Mondi wrote:\n> > > On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > The DRM helper picks the first DRM card that it can open. On platforms\n> > > > that have a standalone GPU, this risks selecting a device corresponding\n> > > > to the GPU instead of the display controller. Fix this by skipping\n> > > > devices that don't support the KMS atomic API. Some legacy display\n> > > > controllers would be skipped as well, but libcamera doesn't run on those\n> > > > systems anyway, and the DRM helper doesn't support the legacy KMS\n> > > > modeset API in any case.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/cam/drm.cpp | 31 +++++++++++++++++++++++--------\n> > > > 1 file changed, 23 insertions(+), 8 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > index b0602c942853..b2a6b0bba9e8 100644\n> > > > --- a/src/cam/drm.cpp\n> > > > +++ b/src/cam/drm.cpp\n> > > > @@ -430,7 +430,8 @@ int Device::init()\n> > > > int Device::openCard()\n> > > > {\n> > > > const std::string dirName = \"/dev/dri/\";\n> > > > - int ret = -ENOENT;\n> > > > + bool found = false;\n> > > > + int ret;\n> > > >\n> > > > /*\n> > > > * Open the first DRM/KMS device beginning with /dev/dri/card. The\n> > > > @@ -449,24 +450,38 @@ int Device::openCard()\n> > > > }\n> > > >\n> > > > for (struct dirent *res; (res = readdir(folder));) {\n> > > > + uint64_t cap;\n> > > > +\n> > > > if (strncmp(res->d_name, \"card\", 4))\n> > > > continue;\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> > > > + if (fd_ < 0) {\n> > > > + ret = -errno;\n> > >\n> > > As we don't propagate errors up, can't you use errno directly ?\n> >\n> > The first calls to operator<<() may modify the value of errno before the\n> > strerror() call is evaluated. See\n> > https://en.cppreference.com/w/cpp/language/eval_order:\n> >\n> > \"Order of evaluation of any part of any expression, including order of\n> > evaluation of function arguments is unspecified (with some exceptions\n> > listed below). The compiler can evaluate operands and other\n> > subexpressions in any order, and may choose another order when the same\n> > expression is evaluated again.\"\n> \n> Yeah, I think what Laurent did with the extra variable is safer, just\n> in case errno changes.\n> \n> > > > + std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > > > + << strerror(-ret) << std::endl;\n> > > > + continue;\n> > > > }\n> > > >\n> > > > - ret = -errno;\n> > > > - std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > > > - << strerror(-ret) << std::endl;\n> > > > + /*\n> > > > + * Skip devices that don't support the atomic API, to avoid\n> > > > + * selecting a DRM device corresponding to a GPU.\n> > > > + */\n> > > > + ret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);\n> > >\n> > > This is the only reference I found of drmGetCap:\n> > > https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/\n> > >\n> > > Can it return a negative value ?\n> \n> It's important to read the source code on gitlab for drm, because they\n> accept gitlab Merge Requests as well as submissions via mailing list\n> (like Laurent alluded to). My last patch for drm was accepted via\n> gitlab, so it probably doesn't appear on the mailing list as an\n> example. So looking at:\n> \n> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c\n> \n> I think what's most correct is `if (ret || !cap)` from looking at the\n> outermost drmGetCap function, the cap.value only gets set if we return\n> 0 (and as a secondary check, make sure capability is set to non-zero).\n> Personally I think this is better as the reader can identify what is\n> intended without going into kernel-space code. I also saw this logic\n> in Chromium OS (well they do the inverse):\n> \n> https://chromium.googlesource.com/chromiumos/platform/frecon/+/refs/heads/main/drm.c\n\nIt's interesting that they have made the exact same mistake as I did :-)\nDRM_CLIENT_CAP_ATOMIC is a flag for the drmSetClientCap() function, not\ndrmGetCap(). It only works by chance.\n\n> with this change:\n> \n> Reviewed-by: Eric Curtin \n> \n> >\n> > https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L1383 for\n> > the implementation of the function, which just calls DRM_IOCTL_GET_CAP.\n> > That ends up in drm_getcap() on the kernel side, which, for\n> > DRM_CLIENT_CAP_ATOMIC, ... works by chance, as DRM_IOCTL_GET_CAP take a\n> > DRM_CAP_* argument, not a DRM_CLIENT_CAP_* argument.\n> > DRM_CLIENT_CAP_ATOMIC is equal to 3, which is\n> > DRM_CAP_DUMB_PREFERRED_DEPTH. I'll fix this to use DRM_CAP_DUMB_BUFFER.\n> > It doesn't matter much what cap we read, the important part is to pick\n> > one that returns an error for non-KMS drivers.\n> >\n> > > > + if (ret < 0) {\n> > > > + drmClose(fd_);\n> > > > + fd_ = -1;\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > + found = true;\n> > > > + break;\n> > > > }\n> > > >\n> > > > closedir(folder);\n> > > >\n> > > > - return ret;\n> > > > + return found ? 0 : -ENOENT;\n> > > > }\n> > > >\n> > > > int Device::getResources()","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 8FFA7BD16B\n\tfor ;\n\tWed, 28 Sep 2022 10:09:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7F20622A1;\n\tWed, 28 Sep 2022 12:09:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 246AD6224C\n\tfor ;\n\tWed, 28 Sep 2022 12:09:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4514F47C;\n\tWed, 28 Sep 2022 12:09:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664359781;\n\tbh=lxavoLyRGJT/IYF53L+YQIPhyfXgwx/URU08LW5/ngY=;\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=ZztyyHoS8f6odwUGAFd+/VDhPbXSTbikj5D+C3wG4dde5EvG0/mh2iOcYgRTsK/uQ\n\tki+ztuLarmuc5FgmpQZp92y0N4bp+ntYo4aju3rZM6mQIS/xTnGQ6dc1HBZnGNjxhR\n\tWhjiGqi+1b5cugpNsdaBCEHQRHR84OpRyx9gGsHJFNlAcT+101Kf5v45SEWWSuD5av\n\tFDikFcoVFkcJayfqbE0NYSPyjbdmxOsLkK4bYLo91qiV6lt9M8uUREyt1RdL+m824I\n\te7VQBnAiVbBy2TTD6SP57JdWLCayVmEk6qJ7UjHoDRV4k8liHhtvvqmJrEv3fWLGWE\n\tibFuWx8z/U8fA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664359780;\n\tbh=lxavoLyRGJT/IYF53L+YQIPhyfXgwx/URU08LW5/ngY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wTq8bKVQWBLrRceU3Zl/deRoz946gL53/lR4Q7mV/y9eN69bt7zNwauyEiq2K866x\n\tkaavvRISUaiGgwiQjQvYZ9tSooegekk68gxFi2nzrzJaKe+UlimnSkDX6sIKBPycri\n\ty2TLGROSbcdLeGj9U7CbqTptKUo33eQUyDmb9yxQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wTq8bKVQ\"; dkim-atps=neutral","Date":"Wed, 28 Sep 2022 13:09:39 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220927232628.7544-1-laurent.pinchart@ideasonboard.com>\n\t<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>\n\t\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: Skip DRM devices not\n\tcapable of supporting the atomic API","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25159,"web_url":"https://patchwork.libcamera.org/comment/25159/","msgid":"","date":"2022-09-28T10:35:56","subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Wed, 28 Sept 2022 at 11:09, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Wed, Sep 28, 2022 at 10:57:29AM +0100, Eric Curtin wrote:\n> > On Wed, 28 Sept 2022 at 09:52, Laurent Pinchart wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On Wed, Sep 28, 2022 at 08:10:02AM +0200, Jacopo Mondi wrote:\n> > > > On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > > The DRM helper picks the first DRM card that it can open. On platforms\n> > > > > that have a standalone GPU, this risks selecting a device corresponding\n> > > > > to the GPU instead of the display controller. Fix this by skipping\n> > > > > devices that don't support the KMS atomic API. Some legacy display\n> > > > > controllers would be skipped as well, but libcamera doesn't run on those\n> > > > > systems anyway, and the DRM helper doesn't support the legacy KMS\n> > > > > modeset API in any case.\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/cam/drm.cpp | 31 +++++++++++++++++++++++--------\n> > > > > 1 file changed, 23 insertions(+), 8 deletions(-)\n> > > > >\n> > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > > > index b0602c942853..b2a6b0bba9e8 100644\n> > > > > --- a/src/cam/drm.cpp\n> > > > > +++ b/src/cam/drm.cpp\n> > > > > @@ -430,7 +430,8 @@ int Device::init()\n> > > > > int Device::openCard()\n> > > > > {\n> > > > > const std::string dirName = \"/dev/dri/\";\n> > > > > - int ret = -ENOENT;\n> > > > > + bool found = false;\n> > > > > + int ret;\n> > > > >\n> > > > > /*\n> > > > > * Open the first DRM/KMS device beginning with /dev/dri/card. The\n> > > > > @@ -449,24 +450,38 @@ int Device::openCard()\n> > > > > }\n> > > > >\n> > > > > for (struct dirent *res; (res = readdir(folder));) {\n> > > > > + uint64_t cap;\n> > > > > +\n> > > > > if (strncmp(res->d_name, \"card\", 4))\n> > > > > continue;\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> > > > > + if (fd_ < 0) {\n> > > > > + ret = -errno;\n> > > >\n> > > > As we don't propagate errors up, can't you use errno directly ?\n> > >\n> > > The first calls to operator<<() may modify the value of errno before the\n> > > strerror() call is evaluated. See\n> > > https://en.cppreference.com/w/cpp/language/eval_order:\n> > >\n> > > \"Order of evaluation of any part of any expression, including order of\n> > > evaluation of function arguments is unspecified (with some exceptions\n> > > listed below). The compiler can evaluate operands and other\n> > > subexpressions in any order, and may choose another order when the same\n> > > expression is evaluated again.\"\n> >\n> > Yeah, I think what Laurent did with the extra variable is safer, just\n> > in case errno changes.\n> >\n> > > > > + std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > > > > + << strerror(-ret) << std::endl;\n> > > > > + continue;\n> > > > > }\n> > > > >\n> > > > > - ret = -errno;\n> > > > > - std::cerr << \"Failed to open DRM/KMS device \" << devName << \": \"\n> > > > > - << strerror(-ret) << std::endl;\n> > > > > + /*\n> > > > > + * Skip devices that don't support the atomic API, to avoid\n> > > > > + * selecting a DRM device corresponding to a GPU.\n> > > > > + */\n> > > > > + ret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);\n> > > >\n> > > > This is the only reference I found of drmGetCap:\n> > > > https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/\n> > > >\n> > > > Can it return a negative value ?\n> >\n> > It's important to read the source code on gitlab for drm, because they\n> > accept gitlab Merge Requests as well as submissions via mailing list\n> > (like Laurent alluded to). My last patch for drm was accepted via\n> > gitlab, so it probably doesn't appear on the mailing list as an\n> > example. So looking at:\n> >\n> > https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c\n> >\n> > I think what's most correct is `if (ret || !cap)` from looking at the\n> > outermost drmGetCap function, the cap.value only gets set if we return\n> > 0 (and as a secondary check, make sure capability is set to non-zero).\n> > Personally I think this is better as the reader can identify what is\n> > intended without going into kernel-space code. I also saw this logic\n> > in Chromium OS (well they do the inverse):\n> >\n> > https://chromium.googlesource.com/chromiumos/platform/frecon/+/refs/heads/main/drm.c\n>\n> It's interesting that they have made the exact same mistake as I did :-)\n> DRM_CLIENT_CAP_ATOMIC is a flag for the drmSetClientCap() function, not\n> drmGetCap(). It only works by chance.\n\nSorry yes, I'll wait for this fix even. Yikes...\n\n>\n> > with this change:\n> >\n> > Reviewed-by: Eric Curtin \n> >\n> > >\n> > > https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L1383 for\n> > > the implementation of the function, which just calls DRM_IOCTL_GET_CAP.\n> > > That ends up in drm_getcap() on the kernel side, which, for\n> > > DRM_CLIENT_CAP_ATOMIC, ... works by chance, as DRM_IOCTL_GET_CAP take a\n> > > DRM_CAP_* argument, not a DRM_CLIENT_CAP_* argument.\n> > > DRM_CLIENT_CAP_ATOMIC is equal to 3, which is\n> > > DRM_CAP_DUMB_PREFERRED_DEPTH. I'll fix this to use DRM_CAP_DUMB_BUFFER.\n> > > It doesn't matter much what cap we read, the important part is to pick\n> > > one that returns an error for non-KMS drivers.\n> > >\n> > > > > + if (ret < 0) {\n> > > > > + drmClose(fd_);\n> > > > > + fd_ = -1;\n> > > > > + continue;\n> > > > > + }\n> > > > > +\n> > > > > + found = true;\n> > > > > + break;\n> > > > > }\n> > > > >\n> > > > > closedir(folder);\n> > > > >\n> > > > > - return ret;\n> > > > > + return found ? 0 : -ENOENT;\n> > > > > }\n> > > > >\n> > > > > int Device::getResources()\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 09D37BD16B\n\tfor ;\n\tWed, 28 Sep 2022 10:36:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5277E622A5;\n\tWed, 28 Sep 2022 12:36:18 +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 7DC526224C\n\tfor ;\n\tWed, 28 Sep 2022 12:36:16 +0200 (CEST)","from mail-vk1-f198.google.com (mail-vk1-f198.google.com\n\t[209.85.221.198]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id\n\tus-mta-287-DsMUEoXGMrSPuJVXsbVdOQ-1; Wed, 28 Sep 2022 06:36:13 -0400","by mail-vk1-f198.google.com with SMTP id\n\tx15-20020a1f7c0f000000b003a35ae05d38so4291786vkc.11\n\tfor ;\n\tWed, 28 Sep 2022 03:36:13 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664361378;\n\tbh=nixTGvLtPYJKoSXMYd+8pwUyLEkpMXzeGyJ7PuW3du8=;\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=RT8DiFAjHl0gpXPX4gxAXBUs2sS/J0sT4nfUT5F7FivTz11XIIsHHoyeyxNhBbLMD\n\tzxrtg4X07oldTDUa4pJVrW3+osSTMNf8LUHJU3vH+2O7p16qkX2O6GUbPqL1ecCb7E\n\tDKZWt6THnUFhrTRZxuHfGsChfOZhz/nygFKutv9QnREfw04VHY2VejdNVq6+gYARBB\n\te+4PaFS47j4EvUh+COWwBv5AansQw/KVRIkdJOWMnT/2J0kmDYMsutw/U7JgKmULtG\n\tsPdviEt3NEaVfc7tt1HVyaAJb3BanTWES3PD11ShX7adRyPH/XOeyRmqhLhEmsCecD\n\tIvP38BasS6S2w==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1664361375;\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=KnqYmE9F9q4KdOytxbAVKtJFhPPFr53cb9GNQSC6NZk=;\n\tb=atsj0VPHQ0SrOXL8b3iHa1dOVf7/DDyFnSuAsbw3vAZYQ09Ont+8SkSmkgqoOmfF4waQeC\n\tLvW/aOH/CLDe04oLFRVjrTx6mODXmTKr6qj5QVMXMzadhhdCLpru5desaxuQ3fFrYPWZBK\n\tlkQXxkSlnjq+N6B/8c00uV5eCuTB9sk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"atsj0VPH\"; \n\tdkim-atps=neutral","X-MC-Unique":"DsMUEoXGMrSPuJVXsbVdOQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=KnqYmE9F9q4KdOytxbAVKtJFhPPFr53cb9GNQSC6NZk=;\n\tb=uL7I9L7NUV6DNieVdpYUiVKMFFbzRvs46mVtUsZ6tqCyBoHTytQkApHMetFpn5DZ9z\n\tCsZBVDS4TQL6uPCrXb2diQd4P53jfALbc7uT6IeQqqSKhmMaBoBmwL58o7lGWOMQjUNp\n\tkAXMRf3P3SYdFkm+PeYYa+Lbw/TQtMTqVScSxige7vZWDzDfu7jqvW3TXEjHgXoXvJ7J\n\txCnmLCvRRQPZGkATQnQKhY2RuHoGHseI8sr9X0kglYp4sWzpti+Mf3G/tzrFH1V10DX3\n\tumt4HMdrImDDy18dxFzA2bexAPcqnq7xY+DKmbOadK/yusaDj2xFrA9snp6xxe5cq0YM\n\tDD+A==","X-Gm-Message-State":"ACrzQf3IBvjsoSeWnQ4LXxZlvKu7uHL1ZxiQpaML7OnaOyT0D0Qfe/j7\n\tyL1Wu46TMv9ai63T7uMaR9+VpWdk+Zkyr95CqTEdCQo132wshisEqhRs8oMDO8vI6cSte46q/T5\n\tahrv1eSAupAXc62ljBQUvlRaUPl2OoPvwdbfZlhYSe2sjywTArA==","X-Received":["by 2002:a1f:26d8:0:b0:39e:e116:59b8 with SMTP id\n\tm207-20020a1f26d8000000b0039ee11659b8mr14376736vkm.36.1664361373235; \n\tWed, 28 Sep 2022 03:36:13 -0700 (PDT)","by 2002:a1f:26d8:0:b0:39e:e116:59b8 with SMTP id\n\tm207-20020a1f26d8000000b0039ee11659b8mr14376729vkm.36.1664361372896;\n\tWed, 28 Sep 2022 03:36:12 -0700 (PDT)"],"X-Google-Smtp-Source":"AMsMyM5rUvx2S9jP0cXZ8+YwcnPYBJoihGjAc14eNnG4fbK8jv9PttzaiWc8kgOuTg9bP85Qp+yI8Fi8oI+pL6IBp28=","MIME-Version":"1.0","References":"<20220927232628.7544-1-laurent.pinchart@ideasonboard.com>\n\t<20220928061002.ratyex3v3qp4lhs3@uno.localdomain>\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 28 Sep 2022 11:35:56 +0100","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: drm: Skip DRM devices not\n\tcapable of supporting the atomic API","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]