[libcamera-devel] [PATCH v3 8/8] cam: Add support for viewfinder through DRM/KMS

[Kieran Bingham]

On 04/08/2021 13:43, Laurent Pinchart wrote:
> Use the KMSSink class to display the viewfinder stream, if any, through
> DRM/KMS. The output connector is selected through the new -D/--display
> argument.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

I'm weary of one small issue with this patch.

cam allocates buffers and requests based on the size from libcamera
suggested by libcamera, while this series introduces a requirement for a
minimum of at least 3 requests.

I 'think' that will be handled better by Nicolas' series where we can
handle allocations with the applciation having more control of setting
some minumums.

Is there anything needed to be handled here? I suspect internally
libcamera is always returning a buffer count of 4 or something most
likely at the moment - so its' proabbly a non-issue until Nicolas'
series starts to introduce MinimumRequests ...

So ... my concerns are likely to be both caused and handled by an out of
tree series and thus...




> ---
> Changes since v2:
> 
> - Add compile guard around option check
> 
> Changes since v1:
> 
> - Validate display option in CamApp::init()
> ---
>  src/cam/camera_session.cpp | 30 ++++++++++++++++++++++++++++++
>  src/cam/main.cpp           |  6 ++++++
>  src/cam/main.h             |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index f34a6ed55ad7..60d640f2b15c 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -16,6 +16,9 @@
>  #include "camera_session.h"
>  #include "event_loop.h"
>  #include "file_sink.h"
> +#ifdef HAVE_KMS
> +#include "kms_sink.h"
> +#endif
>  #include "main.h"
>  #include "stream_options.h"
>  
> @@ -66,6 +69,28 @@ CameraSession::CameraSession(CameraManager *cm,
>  
>  	bool strictFormats = options_.isSet(OptStrictFormats);
>  
> +#ifdef HAVE_KMS
> +	if (options_.isSet(OptDisplay)) {
> +		if (options_.isSet(OptFile)) {
> +			std::cerr << "--display and --file options are mutually exclusive"
> +				  << std::endl;
> +			return;
> +		}
> +
> +		if (roles.size() != 1) {
> +			std::cerr << "Display doesn't support multiple streams"
> +				  << std::endl;

Do you also protect/prevent multiple cameras trying to display at the
same time?

(now that we have multi-cam support in cam...?)

If that's going to be OK, or when handled if it's a problem:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +			return;
> +		}
> +
> +		if (roles[0] != StreamRole::Viewfinder) {
> +			std::cerr << "Display requires a viewfinder stream"
> +				  << std::endl;
> +			return;
> +		}
> +	}
> +#endif
> +
>  	switch (config->validate()) {
>  	case CameraConfiguration::Valid:
>  		break;
> @@ -161,6 +186,11 @@ int CameraSession::start()
>  
>  	camera_->requestCompleted.connect(this, &CameraSession::requestComplete);
>  
> +#ifdef HAVE_KMS
> +	if (options_.isSet(OptDisplay))
> +		sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString());
> +#endif
> +
>  	if (options_.isSet(OptFile)) {
>  		if (!options_[OptFile].toString().empty())
>  			sink_ = std::make_unique<FileSink>(options_[OptFile]);
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 34cbc3229563..c7f664b903e0 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -132,6 +132,12 @@ int CamApp::parseOptions(int argc, char *argv[])
>  			 "Capture until interrupted by user or until <count> frames captured",
>  			 "capture", ArgumentOptional, "count", false,
>  			 OptCamera);
> +#ifdef HAVE_KMS
> +	parser.addOption(OptDisplay, OptionString,
> +			 "Display viewfinder through DRM/KMS on specified connector",
> +			 "display", ArgumentOptional, "connector", false,
> +			 OptCamera);
> +#endif
>  	parser.addOption(OptFile, OptionString,
>  			 "Write captured frames to disk\n"
>  			 "If the file name ends with a '/', it sets the directory in which\n"
> diff --git a/src/cam/main.h b/src/cam/main.h
> index d22451f59817..1c2fab763698 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -10,6 +10,7 @@
>  enum {
>  	OptCamera = 'c',
>  	OptCapture = 'C',
> +	OptDisplay = 'D',
>  	OptFile = 'F',
>  	OptHelp = 'h',
>  	OptInfo = 'I',
>