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

[Kieran Bingham]

Quoting Eric Curtin (2021-10-28 16:26:53)
> Hi Laurent,
> 
> On Wed, 27 Oct 2021 at 22:13, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi Eric,
> >
> > Thank you for the patch.
> >
> > On Wed, Oct 27, 2021 at 11:04:41AM +0100, Eric Curtin wrote:
> > > xrand --query isn't always available and does not always display all
> > > conneted connectors.
> >
> > Stupid questions, couldn't connectors also be listed through sysfs ?
> >
> > for conn in /sys/class/drm/card*-* ; do status=$(cat $conn/status) ; if [ $status == connected ] ; then echo $conn ; fi ; done
> >
> > I'm not opposed to adding this feature to the cam application, but I
> > wonder if it really belongs here, especially if we consider that a next
> > step would be to print supported modes.

I think it's helpful to have it in cam. Cam has support to render on a
DRM device, but you have to 'know' what device. This is a nice
convenience helper to those who do not know other ways to interogate
DRM.



> 
> Printing modes is where I was gonna do next. I only learned about
> /sys/class/drm/card now
> as a new guy to DRM, I think it would be useful for those who aren't
> familiar with /sys
> directory structure for drm. Finding documentation for this can be
> hard and a tool is easier to maintain anyway.
> 
> Are you thinking maybe a separate binary called drm-info? (drm-utils
> is a taken name by a
> package that is seems not so useful in the context of the cam
> application), etc? We list all
> the equivalent cam information with the "cam -I -c1", it can be hard
> to gather the equivalent
> display info and if we can get them both at the same place, same
> binary, would be nice!
> 
> >
> > > 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");
> > >  #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();
> >
> > As Umang mentioned, errors should be checked.
> >
> > > +             for (const DRM::Connector &conn : dev.connectors()) {
> > > +                     if (conn.status() == DRM::Connector::Connected) {
> > > +                             std::cout << conn.name() << std::endl;
> > > +                     }
> >
> > No need for curly braces in the inner if.

I would be tempted to put any DRM specific code into src/cam/drm.cpp
though, and try to reduce the code that is added to main.cpp.


> >
> > > +             }
> > > +     }
> > > +#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__ */
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> 
> Is mise le meas/Regards,
> 
> Eric Curtin
>