[{"id":20628,"web_url":"https://patchwork.libcamera.org/comment/20628/","msgid":"<163549794136.14824.960674854343621153@Monstersaurus>","date":"2021-10-29T08:59:01","subject":"Re: [libcamera-devel] [PATCH] cam: Add option to list displays","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Eric Curtin (2021-10-28 16:48:07)\n> So the user does not have to be familiar with internals of\n> /sys/class/drm/. \"xrand --query\" names aren't consistent with DRM\n> names and depend on an X server running.\n> \n> This --list-displays option will show the names of the available\n> displays and whether each display's status is connected, disconnected\n> or unknown.\n> \n> Signed-off-by: Eric Curtin \n> ---\n> src/cam/drm.cpp | 3 +++\n> src/cam/drm.h | 2 ++\n> src/cam/main.cpp | 28 +++++++++++++++++++++++-----\n> src/cam/main.h | 3 +++\n> 4 files changed, 31 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> index f2530091..444d639d 100644\n> --- a/src/cam/drm.cpp\n> +++ b/src/cam/drm.cpp\n> @@ -190,15 +190,18 @@ Connector::Connector(Device *dev, const drmModeConnector *connector)\n> switch (connector->connection) {\n> case DRM_MODE_CONNECTED:\n> status_ = Status::Connected;\n> + status_str_ = \"connected\";\n> break;\n> \n> case DRM_MODE_DISCONNECTED:\n> status_ = Status::Disconnected;\n> + status_str_ = \"disconnected\";\n> break;\n> \n> case DRM_MODE_UNKNOWNCONNECTION:\n> default:\n> status_ = Status::Unknown;\n> + status_str_ = \"unknown\";\n> break;\n> }\n> \n> diff --git a/src/cam/drm.h b/src/cam/drm.h\n> index 0b88f9a3..92acd826 100644\n> --- a/src/cam/drm.h\n> +++ b/src/cam/drm.h\n> @@ -187,6 +187,7 @@ public:\n> const std::string &name() const { return name_; }\n> \n> Status status() const { return status_; }\n> + const std::string &status_str() const { return status_str_; }\n> \n> const std::vector &encoders() const { return encoders_; }\n> const std::vector &modes() const { return modes_; }\n> @@ -194,6 +195,7 @@ public:\n> private:\n> uint32_t type_;\n> std::string name_;\n> + std::string status_str_;\n> Status status_;\n> std::vector encoders_;\n> std::vector modes_;\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index c7f664b9..cd1ccf2f 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -15,6 +15,11 @@\n> #include \n> \n> #include \"camera_session.h\"\n> +\n> +#ifdef HAVE_KMS\n> +#include \"drm.h\"\n> +#endif\n> +\n> #include \"event_loop.h\"\n> #include \"main.h\"\n> #include \"options.h\"\n> @@ -137,6 +142,7 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \"Display viewfinder through DRM/KMS on specified connector\",\n> \"display\", ArgumentOptional, \"connector\", false,\n> OptCamera);\n> + parser.addOption(OptListDisplays, OptionNone, \"List all displays\", \"list-displays\");\n\nOptListDisplays isn't defined if HAVE_KMS isn't enabled.\nBut I think it should be, and we shouldn't call parser.addOption if we\ndon't support it.\n\nI'd prefer that to be a C style check that the compiler could take out\nrather than pre-processor ifdefs:\n\n\t if (haveKMS())\n\t \tparser.addOption(...)\n\nBut that will require some rework in drm.h to enable it to be included\nwhen HAVE_KMS isn't set.\n\nIs that something you would be interested in tackling?\n\n> #endif\n> parser.addOption(OptFile, OptionString,\n> \"Write captured frames to disk\\n\"\n> @@ -202,7 +208,19 @@ int CamApp::run()\n> }\n> }\n> \n> - /* 2. Create the camera sessions. */\n> +#ifdef HAVE_KMS\n> + /* 2. List all displays. */\n> + if (options_.isSet(OptListDisplays)) {\n\nIf the drm.h header supports !HAVE_KMS it could define a static function\nfor a call like DRM::listDisplays();\n\nThis would be a no-op with !HAVE_KMS, and implement the DRM\nfunctionality otherwise...\n\nThat would minimize the DRM'ness in this main.cpp.\n\n> + DRM::Device dev;\n> + if (dev.init() >= 0) {\n> + std::cout << \"Available displays:\" << std::endl;\n> + for (const DRM::Connector &conn : dev.connectors())\n> + std::cout << \"name: \" << conn.name() << \" status: \" << conn.status_str() << std::endl;\n> + }\n> + }\n> +#endif\n> +\n> + /* 3. Create the camera sessions. */\n> std::vector> sessions;\n> \n> if (options_.isSet(OptCamera)) {\n> @@ -229,7 +247,7 @@ int CamApp::run()\n> }\n> }\n> \n> - /* 3. Print camera information. */\n> + /* 4. Print camera information. */\n> if (options_.isSet(OptListControls) ||\n> options_.isSet(OptListProperties) ||\n> options_.isSet(OptInfo)) {\n> @@ -243,7 +261,7 @@ int CamApp::run()\n> }\n> }\n> \n> - /* 4. Start capture. */\n> + /* 5. Start capture. */\n> for (const auto &session : sessions) {\n> if (!session->options().isSet(OptCapture))\n> continue;\n> @@ -257,7 +275,7 @@ int CamApp::run()\n> loopUsers_++;\n> }\n> \n> - /* 5. Enable hotplug monitoring. */\n> + /* 6. Enable hotplug monitoring. */\n> if (options_.isSet(OptMonitor)) {\n> std::cout << \"Monitoring new hotplug and unplug events\" << std::endl;\n> std::cout << \"Press Ctrl-C to interrupt\" << std::endl;\n> @@ -271,7 +289,7 @@ int CamApp::run()\n> if (loopUsers_)\n> loop_.exec();\n> \n> - /* 6. Stop capture. */\n> + /* 7. Stop capture. */\n> for (const auto &session : sessions) {\n> if (!session->options().isSet(OptCapture))\n> continue;\n> diff --git a/src/cam/main.h b/src/cam/main.h\n> index 1c2fab76..72050d27 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -21,6 +21,9 @@ enum {\n> OptListControls = 256,\n> OptStrictFormats = 257,\n> OptMetadata = 258,\n> +#ifdef HAVE_KMS\n\nI think OptListDisplays should always be set. Even if !HAVE_KMS.\nThe number is reserved, so nothing else will be using it.\n\n--\nKieran\n\n\n> + OptListDisplays = 259\n> +#endif\n> };\n> \n> #endif /* __CAM_MAIN_H__ */\n> -- \n> 2.31.1\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 5D621BDB1C\n\tfor ;\n\tFri, 29 Oct 2021 08:59:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2B60600BB;\n\tFri, 29 Oct 2021 10:59:05 +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 495A7600B4\n\tfor ;\n\tFri, 29 Oct 2021 10:59:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D672A3E5;\n\tFri, 29 Oct 2021 10:59:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MnieChSi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635497943;\n\tbh=QxwFA8WmZjVysOWIX2a6u5KEBgXM+uI0SZIXsg/DUI4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=MnieChSi/xVn0VbXarMarj4xNMEHs0u9j7711Wv/Of2lRWjbrcfsBgm6evSdOeFI9\n\tSYl0JwAUdkwLIrDkz65K9isI+EtnsnS46pwxbBFApi+uyiiiOniDxYFG1Mjuo++biX\n\tIZeZ+ByL1MEYqwMv0BwY5mLedFjd9CDT0td39q2M=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028154807.36128-1-ecurtin@redhat.com>","References":"<20211028154807.36128-1-ecurtin@redhat.com>","From":"Kieran Bingham ","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Date":"Fri, 29 Oct 2021 09:59:01 +0100","Message-ID":"<163549794136.14824.960674854343621153@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] cam: Add option to list displays","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20634,"web_url":"https://patchwork.libcamera.org/comment/20634/","msgid":"","date":"2021-10-29T09:39:57","subject":"Re: [libcamera-devel] [PATCH] cam: Add option to list displays","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"Hi Kieran,\n\nOn Fri, 29 Oct 2021 at 09:59, Kieran Bingham\n wrote:\n>\n> Quoting Eric Curtin (2021-10-28 16:48:07)\n> > So the user does not have to be familiar with internals of\n> > /sys/class/drm/. \"xrand --query\" names aren't consistent with DRM\n> > names and depend on an X server running.\n> >\n> > This --list-displays option will show the names of the available\n> > displays and whether each display's status is connected, disconnected\n> > or unknown.\n> >\n> > Signed-off-by: Eric Curtin \n> > ---\n> > src/cam/drm.cpp | 3 +++\n> > src/cam/drm.h | 2 ++\n> > src/cam/main.cpp | 28 +++++++++++++++++++++++-----\n> > src/cam/main.h | 3 +++\n> > 4 files changed, 31 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > index f2530091..444d639d 100644\n> > --- a/src/cam/drm.cpp\n> > +++ b/src/cam/drm.cpp\n> > @@ -190,15 +190,18 @@ Connector::Connector(Device *dev, const drmModeConnector *connector)\n> > switch (connector->connection) {\n> > case DRM_MODE_CONNECTED:\n> > status_ = Status::Connected;\n> > + status_str_ = \"connected\";\n> > break;\n> >\n> > case DRM_MODE_DISCONNECTED:\n> > status_ = Status::Disconnected;\n> > + status_str_ = \"disconnected\";\n> > break;\n> >\n> > case DRM_MODE_UNKNOWNCONNECTION:\n> > default:\n> > status_ = Status::Unknown;\n> > + status_str_ = \"unknown\";\n> > break;\n> > }\n> >\n> > diff --git a/src/cam/drm.h b/src/cam/drm.h\n> > index 0b88f9a3..92acd826 100644\n> > --- a/src/cam/drm.h\n> > +++ b/src/cam/drm.h\n> > @@ -187,6 +187,7 @@ public:\n> > const std::string &name() const { return name_; }\n> >\n> > Status status() const { return status_; }\n> > + const std::string &status_str() const { return status_str_; }\n> >\n> > const std::vector &encoders() const { return encoders_; }\n> > const std::vector &modes() const { return modes_; }\n> > @@ -194,6 +195,7 @@ public:\n> > private:\n> > uint32_t type_;\n> > std::string name_;\n> > + std::string status_str_;\n> > Status status_;\n> > std::vector encoders_;\n> > std::vector modes_;\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index c7f664b9..cd1ccf2f 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -15,6 +15,11 @@\n> > #include \n> >\n> > #include \"camera_session.h\"\n> > +\n> > +#ifdef HAVE_KMS\n> > +#include \"drm.h\"\n> > +#endif\n> > +\n> > #include \"event_loop.h\"\n> > #include \"main.h\"\n> > #include \"options.h\"\n> > @@ -137,6 +142,7 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \"Display viewfinder through DRM/KMS on specified connector\",\n> > \"display\", ArgumentOptional, \"connector\", false,\n> > OptCamera);\n> > + parser.addOption(OptListDisplays, OptionNone, \"List all displays\", \"list-displays\");\n>\n> OptListDisplays isn't defined if HAVE_KMS isn't enabled.\n> But I think it should be, and we shouldn't call parser.addOption if we\n> don't support it.\n\nThe diff doesn't show it, but this is in an \"#ifdef HAVE_KMS\" block.\nJust to be explicit.\nshould I take out both OptListDisplays and OptDisplay out of this\n\"#ifdef HAVE_KMS\"\nblock?\n\n>\n> I'd prefer that to be a C style check that the compiler could take out\n> rather than pre-processor ifdefs:\n>\n> if (haveKMS())\n> parser.addOption(...)\n>\n> But that will require some rework in drm.h to enable it to be included\n> when HAVE_KMS isn't set.\n>\n> Is that something you would be interested in tackling?\n\nYeah maybe as a separate patch. To play devils advocate, we'd have two different\ntechniques then right? I think we would still need the #ifdef's for\nthose who'd like to\nbuild cam with libdrm and those who want to build without? Is this inconsistency\nworth it? It's kind of nice to be able to do a:\n\n 'grep -r HAVE_KMS'\n\nand find all the code that is required for DRM/KMS functionality. This\nwould become:\n\n'grep -r \"HAVE_KMS\\|haveKMS\"'\n\nthen, not as obvious.\n\n>\n> > #endif\n> > parser.addOption(OptFile, OptionString,\n> > \"Write captured frames to disk\\n\"\n> > @@ -202,7 +208,19 @@ int CamApp::run()\n> > }\n> > }\n> >\n> > - /* 2. Create the camera sessions. */\n> > +#ifdef HAVE_KMS\n> > + /* 2. List all displays. */\n> > + if (options_.isSet(OptListDisplays)) {\n>\n> If the drm.h header supports !HAVE_KMS it could define a static function\n> for a call like DRM::listDisplays();\n>\n\nI was literally about to click send on an email that said the following:\n\nSomething like:\n\n if (options_.isSet(OptListDisplays)) {\n DRM::Device dev;\n dev.listDisplays()\n }\n\n?\n\nBut now I know exactly what you want. The static function should be nicer.\n\n> This would be a no-op with !HAVE_KMS, and implement the DRM\n> functionality otherwise...\n>\n> That would minimize the DRM'ness in this main.cpp.\n>\n> > + DRM::Device dev;\n> > + if (dev.init() >= 0) {\n> > + std::cout << \"Available displays:\" << std::endl;\n> > + for (const DRM::Connector &conn : dev.connectors())\n> > + std::cout << \"name: \" << conn.name() << \" status: \" << conn.status_str() << std::endl;\n> > + }\n> > + }\n> > +#endif\n> > +\n> > + /* 3. Create the camera sessions. */\n> > std::vector> sessions;\n> >\n> > if (options_.isSet(OptCamera)) {\n> > @@ -229,7 +247,7 @@ int CamApp::run()\n> > }\n> > }\n> >\n> > - /* 3. Print camera information. */\n> > + /* 4. Print camera information. */\n> > if (options_.isSet(OptListControls) ||\n> > options_.isSet(OptListProperties) ||\n> > options_.isSet(OptInfo)) {\n> > @@ -243,7 +261,7 @@ int CamApp::run()\n> > }\n> > }\n> >\n> > - /* 4. Start capture. */\n> > + /* 5. Start capture. */\n> > for (const auto &session : sessions) {\n> > if (!session->options().isSet(OptCapture))\n> > continue;\n> > @@ -257,7 +275,7 @@ int CamApp::run()\n> > loopUsers_++;\n> > }\n> >\n> > - /* 5. Enable hotplug monitoring. */\n> > + /* 6. Enable hotplug monitoring. */\n> > if (options_.isSet(OptMonitor)) {\n> > std::cout << \"Monitoring new hotplug and unplug events\" << std::endl;\n> > std::cout << \"Press Ctrl-C to interrupt\" << std::endl;\n> > @@ -271,7 +289,7 @@ int CamApp::run()\n> > if (loopUsers_)\n> > loop_.exec();\n> >\n> > - /* 6. Stop capture. */\n> > + /* 7. Stop capture. */\n> > for (const auto &session : sessions) {\n> > if (!session->options().isSet(OptCapture))\n> > continue;\n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index 1c2fab76..72050d27 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -21,6 +21,9 @@ enum {\n> > OptListControls = 256,\n> > OptStrictFormats = 257,\n> > OptMetadata = 258,\n> > +#ifdef HAVE_KMS\n>\n> I think OptListDisplays should always be set. Even if !HAVE_KMS.\n> The number is reserved, so nothing else will be using it.\n\nYup makes sense.\n\n\n>\n> --\n> Kieran\n>\n>\n> > + OptListDisplays = 259\n> > +#endif\n> > };\n> >\n> > #endif /* __CAM_MAIN_H__ */\n> > --\n> > 2.31.1\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 3B050BDB1C\n\tfor ;\n\tFri, 29 Oct 2021 09:40:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB72F600BB;\n\tFri, 29 Oct 2021 11:40:13 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [216.205.24.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57F9D600B4\n\tfor ;\n\tFri, 29 Oct 2021 11:40:12 +0200 (CEST)","from mail-oo1-f70.google.com (mail-oo1-f70.google.com\n\t[209.85.161.70]) (Using TLS) by relay.mimecast.com with ESMTP id\n\tus-mta-347-NdAmOrOsOPCsCV3mYoX9Ng-1; Fri, 29 Oct 2021 05:40:09 -0400","by mail-oo1-f70.google.com with SMTP id\n\ti1-20020a4a9001000000b002a9c41e0eabso4162970oog.3\n\tfor ;\n\tFri, 29 Oct 2021 02:40:09 -0700 (PDT)"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"WmMGipio\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1635500411;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=G0gdQKiVYxAY4dxVSbBdMoDrDvzIPFyLg/0HRsgyB3A=;\n\tb=WmMGipioNqmx+DhWqPAO3PPpvfevAJg3/LCTtVHSJ06FVU2RAhNZ2GHxF8OQZ/NviW3ftL\n\ttu5NzyTi/bgAl4s/vPGLLsL1g+JGWoocMInLhqdm9eTKMlHbpKyL79uVubKkRvCv0ybJqK\n\t6w10bgZoTH//kjTK/3t9pvAqIpXoi6E=","X-MC-Unique":"NdAmOrOsOPCsCV3mYoX9Ng-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;\n\tbh=G0gdQKiVYxAY4dxVSbBdMoDrDvzIPFyLg/0HRsgyB3A=;\n\tb=E9lAxvd9cFtTxXWRAF5PhpARoN5qyQWoYhuO9fleUiyNSKILLga0F/Ljhy5rc/l3es\n\ttn+BsTStuFBbOD0sONY7RTg/q4xYL8JaSRzeh9/ZqV4lNpj2FhnMtcMmKW9ArH4r+mhe\n\tnYqLlzN7DyaVg2KKmBB91MnEgVvVgBc8bCeV6WfMcdp8nUMjZqLmrNaE0pwsaBGzVJZH\n\tM1uoHUbinXrNERDNn+1c+M8ONTr+LfzELyT0tqwS/u2GqX90Kka95d5byzAkX0jWIoMB\n\t34t0ye5Q6b3URQV3Uhs28reFU3FDfcjr04cQmWBBdN8SWi2IdaYxj/ZuN0nmdBjNMvfi\n\td23A==","X-Gm-Message-State":"AOAM531uOi+NpQT1iPtA2wZwij1uczLXVc/Vs3RgkULYq9zY+dF4COcO\n\tcann1CFDVZqm3vRI3c4Cyf0o9U4U8IiXiVOa1tPgFq1B5RoRo8KZx0jt4SSu4LXXRjXKlSe6DQi\n\tffJF7ou8gxGmkNbFuKWmkmTz5fuTaTk3YYoGqTmtIH9JCx/Hfxw==","X-Received":["by 2002:aca:b10a:: with SMTP id a10mr7225812oif.74.1635500408700;\n\tFri, 29 Oct 2021 02:40:08 -0700 (PDT)","by 2002:aca:b10a:: with SMTP id a10mr7225798oif.74.1635500408415;\n\tFri, 29 Oct 2021 02:40:08 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJyOR7QoWL1mUxuNFjer+Kj2Tbg2AcLZF88V28FXWT9dI3kmsJ1bPG6IyK8UtXAP73kV1OEgF+J0ZfJX9Kbtyl4=","MIME-Version":"1.0","References":"<20211028154807.36128-1-ecurtin@redhat.com>\n\t<163549794136.14824.960674854343621153@Monstersaurus>","In-Reply-To":"<163549794136.14824.960674854343621153@Monstersaurus>","From":"Eric Curtin ","Date":"Fri, 29 Oct 2021 10:39:57 +0100","Message-ID":"","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] cam: Add option to list displays","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20637,"web_url":"https://patchwork.libcamera.org/comment/20637/","msgid":"<163550597243.40363.11615464835158446562@Monstersaurus>","date":"2021-10-29T11:12:52","subject":"Re: [libcamera-devel] [PATCH] cam: Add option to list displays","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Eric Curtin (2021-10-29 10:39:57)\n> Hi Kieran,\n> \n> On Fri, 29 Oct 2021 at 09:59, Kieran Bingham\n> wrote:\n> >\n> > Quoting Eric Curtin (2021-10-28 16:48:07)\n> > > So the user does not have to be familiar with internals of\n> > > /sys/class/drm/. \"xrand --query\" names aren't consistent with DRM\n> > > names and depend on an X server running.\n> > >\n> > > This --list-displays option will show the names of the available\n> > > displays and whether each display's status is connected, disconnected\n> > > or unknown.\n> > >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/drm.cpp | 3 +++\n> > > src/cam/drm.h | 2 ++\n> > > src/cam/main.cpp | 28 +++++++++++++++++++++++-----\n> > > src/cam/main.h | 3 +++\n> > > 4 files changed, 31 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp\n> > > index f2530091..444d639d 100644\n> > > --- a/src/cam/drm.cpp\n> > > +++ b/src/cam/drm.cpp\n> > > @@ -190,15 +190,18 @@ Connector::Connector(Device *dev, const drmModeConnector *connector)\n> > > switch (connector->connection) {\n> > > case DRM_MODE_CONNECTED:\n> > > status_ = Status::Connected;\n> > > + status_str_ = \"connected\";\n> > > break;\n> > >\n> > > case DRM_MODE_DISCONNECTED:\n> > > status_ = Status::Disconnected;\n> > > + status_str_ = \"disconnected\";\n> > > break;\n> > >\n> > > case DRM_MODE_UNKNOWNCONNECTION:\n> > > default:\n> > > status_ = Status::Unknown;\n> > > + status_str_ = \"unknown\";\n> > > break;\n> > > }\n> > >\n> > > diff --git a/src/cam/drm.h b/src/cam/drm.h\n> > > index 0b88f9a3..92acd826 100644\n> > > --- a/src/cam/drm.h\n> > > +++ b/src/cam/drm.h\n> > > @@ -187,6 +187,7 @@ public:\n> > > const std::string &name() const { return name_; }\n> > >\n> > > Status status() const { return status_; }\n> > > + const std::string &status_str() const { return status_str_; }\n> > >\n> > > const std::vector &encoders() const { return encoders_; }\n> > > const std::vector &modes() const { return modes_; }\n> > > @@ -194,6 +195,7 @@ public:\n> > > private:\n> > > uint32_t type_;\n> > > std::string name_;\n> > > + std::string status_str_;\n> > > Status status_;\n> > > std::vector encoders_;\n> > > std::vector modes_;\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index c7f664b9..cd1ccf2f 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -15,6 +15,11 @@\n> > > #include \n> > >\n> > > #include \"camera_session.h\"\n> > > +\n> > > +#ifdef HAVE_KMS\n> > > +#include \"drm.h\"\n> > > +#endif\n> > > +\n> > > #include \"event_loop.h\"\n> > > #include \"main.h\"\n> > > #include \"options.h\"\n> > > @@ -137,6 +142,7 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > \"Display viewfinder through DRM/KMS on specified connector\",\n> > > \"display\", ArgumentOptional, \"connector\", false,\n> > > OptCamera);\n> > > + parser.addOption(OptListDisplays, OptionNone, \"List all displays\", \"list-displays\");\n> >\n> > OptListDisplays isn't defined if HAVE_KMS isn't enabled.\n> > But I think it should be, and we shouldn't call parser.addOption if we\n> > don't support it.\n> \n> The diff doesn't show it, but this is in an \"#ifdef HAVE_KMS\" block.\n> Just to be explicit.\n> should I take out both OptListDisplays and OptDisplay out of this\n> \"#ifdef HAVE_KMS\"\n> block?\n\nAh, sorry that wasn't visible indeed. Given the option above it though,\nit makes more sense so I should have realised it was already in a\nHAVE_KMS block ;-)\n\n\n> > I'd prefer that to be a C style check that the compiler could take out\n> > rather than pre-processor ifdefs:\n> >\n> > if (haveKMS())\n> > parser.addOption(...)\n> >\n> > But that will require some rework in drm.h to enable it to be included\n> > when HAVE_KMS isn't set.\n> >\n> > Is that something you would be interested in tackling?\n> \n> Yeah maybe as a separate patch. To play devils advocate, we'd have two different\n\nYes, if done, it would be separate.\n\n> techniques then right? I think we would still need the #ifdef's for\n> those who'd like to\n> build cam with libdrm and those who want to build without? Is this inconsistency\n> worth it? It's kind of nice to be able to do a:\n> \n> 'grep -r HAVE_KMS'\n> \n> and find all the code that is required for DRM/KMS functionality. This\n> would become:\n> \n> 'grep -r \"HAVE_KMS\\|haveKMS\"'\n> \n> then, not as obvious.\n\nHrm... that's a good point though.\n\nMy main thought it that I prefer to keep as much code visible to the\ncompiler as possible. Hiding code under ifdefs adds more complexity to\nconfiguration matrixes to validate compilation tests of all\ncombinations of the code.\n\nWith a stubbed haveKMS type check on these, the compiler would validate\nthe code, and see it's always if (0), and optimise it out anyway, but -\nthe code would have been checked and validated by the compiler.\n\n\n> > > #endif\n> > > parser.addOption(OptFile, OptionString,\n> > > \"Write captured frames to disk\\n\"\n> > > @@ -202,7 +208,19 @@ int CamApp::run()\n> > > }\n> > > }\n> > >\n> > > - /* 2. Create the camera sessions. */\n> > > +#ifdef HAVE_KMS\n> > > + /* 2. List all displays. */\n> > > + if (options_.isSet(OptListDisplays)) {\n> >\n> > If the drm.h header supports !HAVE_KMS it could define a static function\n> > for a call like DRM::listDisplays();\n> >\n> \n> I was literally about to click send on an email that said the following:\n> \n> Something like:\n> \n> if (options_.isSet(OptListDisplays)) {\n> DRM::Device dev;\n> dev.listDisplays()\n> }\n> \n> ?\n> \n> But now I know exactly what you want. The static function should be nicer.\n\nChatting with Laurent, I think he's worried about layering violations of\nsome sort, so I suspect we might hear from him on this too, probably\nwith a different opinion to me.\n\n\n> > This would be a no-op with !HAVE_KMS, and implement the DRM\n> > functionality otherwise...\n> >\n> > That would minimize the DRM'ness in this main.cpp.\n> >\n> > > + DRM::Device dev;\n> > > + if (dev.init() >= 0) {\n> > > + std::cout << \"Available displays:\" << std::endl;\n> > > + for (const DRM::Connector &conn : dev.connectors())\n> > > + std::cout << \"name: \" << conn.name() << \" status: \" << conn.status_str() << std::endl;\n> > > + }\n> > > + }\n> > > +#endif\n> > > +\n> > > + /* 3. Create the camera sessions. */\n> > > std::vector> sessions;\n> > >\n> > > if (options_.isSet(OptCamera)) {\n> > > @@ -229,7 +247,7 @@ int CamApp::run()\n> > > }\n> > > }\n> > >\n> > > - /* 3. Print camera information. */\n> > > + /* 4. Print camera information. */\n> > > if (options_.isSet(OptListControls) ||\n> > > options_.isSet(OptListProperties) ||\n> > > options_.isSet(OptInfo)) {\n> > > @@ -243,7 +261,7 @@ int CamApp::run()\n> > > }\n> > > }\n> > >\n> > > - /* 4. Start capture. */\n> > > + /* 5. Start capture. */\n> > > for (const auto &session : sessions) {\n> > > if (!session->options().isSet(OptCapture))\n> > > continue;\n> > > @@ -257,7 +275,7 @@ int CamApp::run()\n> > > loopUsers_++;\n> > > }\n> > >\n> > > - /* 5. Enable hotplug monitoring. */\n> > > + /* 6. Enable hotplug monitoring. */\n> > > if (options_.isSet(OptMonitor)) {\n> > > std::cout << \"Monitoring new hotplug and unplug events\" << std::endl;\n> > > std::cout << \"Press Ctrl-C to interrupt\" << std::endl;\n> > > @@ -271,7 +289,7 @@ int CamApp::run()\n> > > if (loopUsers_)\n> > > loop_.exec();\n> > >\n> > > - /* 6. Stop capture. */\n> > > + /* 7. Stop capture. */\n> > > for (const auto &session : sessions) {\n> > > if (!session->options().isSet(OptCapture))\n> > > continue;\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index 1c2fab76..72050d27 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -21,6 +21,9 @@ enum {\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > OptMetadata = 258,\n> > > +#ifdef HAVE_KMS\n> >\n> > I think OptListDisplays should always be set. Even if !HAVE_KMS.\n> > The number is reserved, so nothing else will be using it.\n> \n> Yup makes sense.\n> \n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> > > + OptListDisplays = 259\n> > > +#endif\n> > > };\n> > >\n> > > #endif /* __CAM_MAIN_H__ */\n> > > --\n> > > 2.31.1\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 12DC0BF415\n\tfor ;\n\tFri, 29 Oct 2021 11:12:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA1A9600BD;\n\tFri, 29 Oct 2021 13:12:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DF7B600B8\n\tfor ;\n\tFri, 29 Oct 2021 13:12:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AEBF8881;\n\tFri, 29 Oct 2021 13:12:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o7tN7lRj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635505974;\n\tbh=7kpNT2T6SYhJQQHFYc4vReJS1vcrPyvMwJLWs4o94yU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=o7tN7lRjxaC2lp5TU5mnxorW0heqUF7SEjMA+N8veLuQ/lb9bEeGqaJiaUEgs9fMx\n\tXJHHoRWp58AkNAlsSKq1t1zDQepigla98YaFPGuSifExlNjlUUy7nF+2wvVWnq8r3M\n\tPF1GtzTLqtQUf85phEwDk9SijzY2URk3fYicWxpM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211028154807.36128-1-ecurtin@redhat.com>\n\t<163549794136.14824.960674854343621153@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Date":"Fri, 29 Oct 2021 12:12:52 +0100","Message-ID":"<163550597243.40363.11615464835158446562@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] cam: Add option to list displays","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]