[libcamera-devel] [PATCH] Add option to list displays
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 28 20:07:41 CEST 2021
On Thu, Oct 28, 2021 at 05:41:31PM +0100, Kieran Bingham wrote:
> Quoting Eric Curtin (2021-10-28 16:26:53)
> > On Wed, 27 Oct 2021 at 22:13, Laurent Pinchart wrote:
> > > 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.
I'll side with the majority then :-)
> > 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!
cam is a tool that is meant to exercise the whole libcamera API, in a
bit of a swiss army knife kind of way. It has a few extra features not
related to cameras as such, including support for DRM/KMS output. I'd
like to keep the focus on the camera side, and not solve every other
side issue that the world may be experiencing. In this specific case,
there's a (in my opinion) not too difficult way to find the connectors
and their states from sysfs, but at the same time the implementation of
a similar feature in cam is not too intrusive (at least so far). I'm
thus OK with it, but if we later add an option to print modes, and then
later something else, I may regret this decision :-)
> > > > 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
More information about the libcamera-devel
mailing list