[libcamera-devel] cam: Add option to list displays
diff mbox series

Message ID 20211028154807.36128-1-ecurtin@redhat.com
State New
Headers show
Series
  • [libcamera-devel] cam: Add option to list displays
Related show

Commit Message

Eric Curtin Oct. 28, 2021, 3:48 p.m. UTC
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@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(-)

Comments

Kieran Bingham Oct. 29, 2021, 8:59 a.m. UTC | #1
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@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
>
Eric Curtin Oct. 29, 2021, 9:39 a.m. UTC | #2
Hi Kieran,

On Fri, 29 Oct 2021 at 09:59, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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@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.

The diff doesn't show it, but this is in an "#ifdef HAVE_KMS" block.
Just to be explicit.
should I take out both OptListDisplays and OptDisplay out of this
"#ifdef HAVE_KMS"
block?

>
> 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?

Yeah maybe as a separate patch. To play devils advocate, we'd have two different
techniques then right? I think we would still need the #ifdef's for
those who'd like to
build cam with libdrm and those who want to build without? Is this inconsistency
worth it? It's kind of nice to be able to do a:

 'grep -r HAVE_KMS'

and find all the code that is required for DRM/KMS functionality. This
would become:

'grep -r "HAVE_KMS\|haveKMS"'

then, not as obvious.

>
> >  #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();
>

I was literally about to click send on an email that said the following:

Something like:

       if (options_.isSet(OptListDisplays)) {
               DRM::Device dev;
               dev.listDisplays()
       }

?

But now I know exactly what you want. The static function should be nicer.

> 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.

Yup makes sense.


>
> --
> Kieran
>
>
> > +       OptListDisplays = 259
> > +#endif
> >  };
> >
> >  #endif /* __CAM_MAIN_H__ */
> > --
> > 2.31.1
> >
>
Kieran Bingham Oct. 29, 2021, 11:12 a.m. UTC | #3
Quoting Eric Curtin (2021-10-29 10:39:57)
> Hi Kieran,
> 
> On Fri, 29 Oct 2021 at 09:59, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > 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@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.
> 
> The diff doesn't show it, but this is in an "#ifdef HAVE_KMS" block.
> Just to be explicit.
> should I take out both OptListDisplays and OptDisplay out of this
> "#ifdef HAVE_KMS"
> block?

Ah, sorry that wasn't visible indeed. Given the option above it though,
it makes more sense so I should have realised it was already in a
HAVE_KMS block ;-)


> > 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?
> 
> Yeah maybe as a separate patch. To play devils advocate, we'd have two different

Yes, if done, it would be separate.

> techniques then right? I think we would still need the #ifdef's for
> those who'd like to
> build cam with libdrm and those who want to build without? Is this inconsistency
> worth it? It's kind of nice to be able to do a:
> 
>  'grep -r HAVE_KMS'
> 
> and find all the code that is required for DRM/KMS functionality. This
> would become:
> 
> 'grep -r "HAVE_KMS\|haveKMS"'
> 
> then, not as obvious.

Hrm... that's a good point though.

My main thought it that I prefer to keep as much code visible to the
compiler as possible. Hiding code under ifdefs adds more complexity to
configuration matrixes to validate compilation tests of all
combinations of the code.

With a stubbed haveKMS type check on these, the compiler would validate
the code, and see it's always if (0), and optimise it out anyway, but -
the code would have been checked and validated by the compiler.


> > >  #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();
> >
> 
> I was literally about to click send on an email that said the following:
> 
> Something like:
> 
>        if (options_.isSet(OptListDisplays)) {
>                DRM::Device dev;
>                dev.listDisplays()
>        }
> 
> ?
> 
> But now I know exactly what you want. The static function should be nicer.

Chatting with Laurent, I think he's worried about layering violations of
some sort, so I suspect we might hear from him on this too, probably
with a different opinion to me.


> > 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.
> 
> Yup makes sense.
> 
> 
> >
> > --
> > Kieran
> >
> >
> > > +       OptListDisplays = 259
> > > +#endif
> > >  };
> > >
> > >  #endif /* __CAM_MAIN_H__ */
> > > --
> > > 2.31.1
> > >
> >
>

Patch
diff mbox series

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");
 #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)) {
+		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
+	OptListDisplays = 259
+#endif
 };
 
 #endif /* __CAM_MAIN_H__ */