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

[Umang Jain]

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)

>   #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__ */