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

[Eric Curtin]

Hi Umang,

On Wed, 27 Oct 2021 at 19:29, Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On 10/27/21 3:34 PM, Eric Curtin wrote:
> > xrand --query isn't always available and does not always display all
> > conneted connectors.
>
> s/conneted/connected/
>
> I believe commit message can be a little bit more descriptive providing
> some context around xrand --query.
>
> Also a comment on the subject, it should be prefixed with `cam: `
>
>      cam: Add option to list displays
>
>
> >
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> >   src/cam/main.cpp | 31 ++++++++++++++++++++++++++-----
> >   src/cam/main.h   |  3 +++
> >   2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index c7f664b9..319cbf4d 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");
>
>
> Can there are displays with status ::Disconnected or ::Unknown, I am not
> sure. If yes, I would reword the description to
>
>          "List all connected displays"
>
> (just a suggestion, if it makes sense to you)

You got me thinking, I might just list all displays connected, disconnected and
unknown. How many displays can there be? Might as well display all,
the user could
always filter via grep.

>
> >   #endif
> >       parser.addOption(OptFile, OptionString,
> >                        "Write captured frames to disk\n"
> > @@ -202,7 +208,22 @@ int CamApp::run()
> >               }
> >       }
> >
> > -     /* 2. Create the camera sessions. */
> > +#ifdef HAVE_KMS
> > +     /* 2. List all displays. */
> > +     if (options_.isSet(OptListDisplays)) {
> > +             std::cout << "Available displays:" << std::endl;
> > +
> > +             DRM::Device dev;
> > +             dev.init();
>
>
> This returns an integer value which should be checked.
>
> > +             for (const DRM::Connector &conn : dev.connectors()) {
> > +                     if (conn.status() == DRM::Connector::Connected) {
> > +                             std::cout << conn.name() << std::endl;
>
>
> I wonder if we should also print the type (conn.type()) along with the
> connector name.

The type is included in the name and deduced from this map, (interestingly
a "DP" type is often physically a HDMI port):

const std::map<uint32_t, const char *> connectorTypeNames{
{ DRM_MODE_CONNECTOR_Unknown, "Unknown" },
{ DRM_MODE_CONNECTOR_VGA, "VGA" },
{ DRM_MODE_CONNECTOR_DVII, "DVI-I" },
{ DRM_MODE_CONNECTOR_DVID, "DVI-D" },
{ DRM_MODE_CONNECTOR_DVIA, "DVI-A" },
{ DRM_MODE_CONNECTOR_Composite, "Composite" },
{ DRM_MODE_CONNECTOR_SVIDEO, "S-Video" },
{ DRM_MODE_CONNECTOR_LVDS, "LVDS" },
{ DRM_MODE_CONNECTOR_Component, "Component" },
{ DRM_MODE_CONNECTOR_9PinDIN, "9-Pin-DIN" },
{ DRM_MODE_CONNECTOR_DisplayPort, "DP" },
{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },
{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },
{ DRM_MODE_CONNECTOR_TV, "TV" },
{ DRM_MODE_CONNECTOR_eDP, "eDP" },
{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
{ DRM_MODE_CONNECTOR_DSI, "DSI" },
{ DRM_MODE_CONNECTOR_DPI, "DPI" },
};

>
> The patch looks to me overall.
>
> > +                     }
> > +             }
> > +     }
> > +#endif
> > +
> > +     /* 3. Create the camera sessions. */
> >       std::vector<std::unique_ptr<CameraSession>> sessions;
> >
> >       if (options_.isSet(OptCamera)) {
> > @@ -229,7 +250,7 @@ int CamApp::run()
> >               }
> >       }
> >
> > -     /* 3. Print camera information. */
> > +     /* 4. Print camera information. */
> >       if (options_.isSet(OptListControls) ||
> >           options_.isSet(OptListProperties) ||
> >           options_.isSet(OptInfo)) {
> > @@ -243,7 +264,7 @@ int CamApp::run()
> >               }
> >       }
> >
> > -     /* 4. Start capture. */
> > +     /* 5. Start capture. */
> >       for (const auto &session : sessions) {
> >               if (!session->options().isSet(OptCapture))
> >                       continue;
> > @@ -257,7 +278,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 +292,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__ */
>

About to resend patch with yours and Laurents suggestions.

Is mise le meas/Regards,

Eric Curtin