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

13202 diff mbox series

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@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
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(+)

Hi Laurent,

Thank you for the patch.

On 8/4/21 6:13 PM, 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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
> 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;
> +			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',

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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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@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',
>

Hi Kieran,

On Thu, Aug 05, 2021 at 01:48:31PM +0100, Kieran Bingham wrote:
> 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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@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...

You're right, we should ensure a minimum number of requests, and you're
also right that we don't need it yet :-) There's ongoing discussion on
this topic in Nícolas's series.

> > ---
> > 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...?)

It's prevented by the fact that it will fail creating the second KMS
sink. In order to display multiple cameras this code will need quite a
bit of rework. I'd be happy to review patches ;-)

> If that's going to be OK, or when handled if it's a problem:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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',