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

[Laurent Pinchart]

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

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 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',

-- 
Regards,

Laurent Pinchart