| Message ID | 20211027100441.18248-1-ecurtin@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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) > #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 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__ */
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. > Signed-off-by: Eric Curtin <ecurtin@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. > + } > + } > +#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__ */
Hi Umang, On Wed, 27 Oct 2021 at 19:29, Umang Jain <umang.jain@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@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
Hi Laurent, On Wed, 27 Oct 2021 at 22:13, Laurent Pinchart <laurent.pinchart@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. 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@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. > > > + } > > + } > > +#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
Quoting Eric Curtin (2021-10-28 16:26:53) > Hi Laurent, > > On Wed, 27 Oct 2021 at 22:13, Laurent Pinchart > <laurent.pinchart@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@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 >
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@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__ */
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(); + for (const DRM::Connector &conn : dev.connectors()) { + if (conn.status() == DRM::Connector::Connected) { + std::cout << conn.name() << std::endl; + } + } + } +#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__ */
xrand --query isn't always available and does not always display all conneted connectors. Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- src/cam/main.cpp | 31 ++++++++++++++++++++++++++----- src/cam/main.h | 3 +++ 2 files changed, 29 insertions(+), 5 deletions(-)