[libcamera-devel] [PATCH] cam: Add option to list displays

[Kieran Bingham]

Quoting Eric Curtin (2021-10-28 16:48:07)
> So the user does not have to be familiar with internals of
> /sys/class/drm/. "xrand --query" names aren't consistent with DRM
> names and depend on an X server running.
> 
> This --list-displays option will show the names of the available
> displays and whether each display's status is connected, disconnected
> or unknown.
> 
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> ---
>  src/cam/drm.cpp  |  3 +++
>  src/cam/drm.h    |  2 ++
>  src/cam/main.cpp | 28 +++++++++++++++++++++++-----
>  src/cam/main.h   |  3 +++
>  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index f2530091..444d639d 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -190,15 +190,18 @@ Connector::Connector(Device *dev, const drmModeConnector *connector)
>         switch (connector->connection) {
>         case DRM_MODE_CONNECTED:
>                 status_ = Status::Connected;
> +               status_str_ = "connected";
>                 break;
>  
>         case DRM_MODE_DISCONNECTED:
>                 status_ = Status::Disconnected;
> +               status_str_ = "disconnected";
>                 break;
>  
>         case DRM_MODE_UNKNOWNCONNECTION:
>         default:
>                 status_ = Status::Unknown;
> +               status_str_ = "unknown";
>                 break;
>         }
>  
> diff --git a/src/cam/drm.h b/src/cam/drm.h
> index 0b88f9a3..92acd826 100644
> --- a/src/cam/drm.h
> +++ b/src/cam/drm.h
> @@ -187,6 +187,7 @@ public:
>         const std::string &name() const { return name_; }
>  
>         Status status() const { return status_; }
> +       const std::string &status_str() const { return status_str_; }
>  
>         const std::vector<const Encoder *> &encoders() const { return encoders_; }
>         const std::vector<Mode> &modes() const { return modes_; }
> @@ -194,6 +195,7 @@ public:
>  private:
>         uint32_t type_;
>         std::string name_;
> +       std::string status_str_;
>         Status status_;
>         std::vector<const Encoder *> encoders_;
>         std::vector<Mode> modes_;
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index c7f664b9..cd1ccf2f 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -15,6 +15,11 @@
>  #include <libcamera/property_ids.h>
>  
>  #include "camera_session.h"
> +
> +#ifdef HAVE_KMS
> +#include "drm.h"
> +#endif
> +
>  #include "event_loop.h"
>  #include "main.h"
>  #include "options.h"
> @@ -137,6 +142,7 @@ int CamApp::parseOptions(int argc, char *argv[])
>                          "Display viewfinder through DRM/KMS on specified connector",
>                          "display", ArgumentOptional, "connector", false,
>                          OptCamera);
> +       parser.addOption(OptListDisplays, OptionNone, "List all displays", "list-displays");

OptListDisplays isn't defined if HAVE_KMS isn't enabled.
But I think it should be, and we shouldn't call parser.addOption if we
don't support it.

I'd prefer that to be a C style check that the compiler could take out
rather than pre-processor ifdefs:

	  if (haveKMS())
	  	parser.addOption(...)

But that will require some rework in drm.h to enable it to be included
when HAVE_KMS isn't set.

Is that something you would be interested in tackling?

>  #endif
>         parser.addOption(OptFile, OptionString,
>                          "Write captured frames to disk\n"
> @@ -202,7 +208,19 @@ int CamApp::run()
>                 }
>         }
>  
> -       /* 2. Create the camera sessions. */
> +#ifdef HAVE_KMS
> +       /* 2. List all displays. */
> +       if (options_.isSet(OptListDisplays)) {

If the drm.h header supports !HAVE_KMS it could define a static function
for a call like DRM::listDisplays();

This would be a no-op with !HAVE_KMS, and implement the DRM
functionality otherwise...

That would minimize the DRM'ness in this main.cpp.

> +               DRM::Device dev;
> +               if (dev.init() >= 0) {
> +                       std::cout << "Available displays:" << std::endl;
> +                       for (const DRM::Connector &conn : dev.connectors())
> +                               std::cout << "name: " << conn.name() << " status: " << conn.status_str() << std::endl;
> +               }
> +       }
> +#endif
> +
> +       /* 3. Create the camera sessions. */
>         std::vector<std::unique_ptr<CameraSession>> sessions;
>  
>         if (options_.isSet(OptCamera)) {
> @@ -229,7 +247,7 @@ int CamApp::run()
>                 }
>         }
>  
> -       /* 3. Print camera information. */
> +       /* 4. Print camera information. */
>         if (options_.isSet(OptListControls) ||
>             options_.isSet(OptListProperties) ||
>             options_.isSet(OptInfo)) {
> @@ -243,7 +261,7 @@ int CamApp::run()
>                 }
>         }
>  
> -       /* 4. Start capture. */
> +       /* 5. Start capture. */
>         for (const auto &session : sessions) {
>                 if (!session->options().isSet(OptCapture))
>                         continue;
> @@ -257,7 +275,7 @@ int CamApp::run()
>                 loopUsers_++;
>         }
>  
> -       /* 5. Enable hotplug monitoring. */
> +       /* 6. Enable hotplug monitoring. */
>         if (options_.isSet(OptMonitor)) {
>                 std::cout << "Monitoring new hotplug and unplug events" << std::endl;
>                 std::cout << "Press Ctrl-C to interrupt" << std::endl;
> @@ -271,7 +289,7 @@ int CamApp::run()
>         if (loopUsers_)
>                 loop_.exec();
>  
> -       /* 6. Stop capture. */
> +       /* 7. Stop capture. */
>         for (const auto &session : sessions) {
>                 if (!session->options().isSet(OptCapture))
>                         continue;
> diff --git a/src/cam/main.h b/src/cam/main.h
> index 1c2fab76..72050d27 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -21,6 +21,9 @@ enum {
>         OptListControls = 256,
>         OptStrictFormats = 257,
>         OptMetadata = 258,
> +#ifdef HAVE_KMS

I think OptListDisplays should always be set. Even if !HAVE_KMS.
The number is reserved, so nothing else will be using it.

--
Kieran


> +       OptListDisplays = 259
> +#endif
>  };
>  
>  #endif /* __CAM_MAIN_H__ */
> -- 
> 2.31.1
>