[libcamera-devel] [PATCH] cam: Add option to list displays
Eric Curtin
ecurtin at redhat.com
Fri Oct 29 11:39:57 CEST 2021
Hi Kieran,
On Fri, 29 Oct 2021 at 09:59, Kieran Bingham
<kieran.bingham at 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 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.
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
> >
>
More information about the libcamera-devel
mailing list