[libcamera-devel] [PATCH 1/2] v4l2: Use SystemDevices properties to identify cameras

[Kieran Bingham]

Quoting Kieran Bingham (2023-06-17 23:16:47)
> Quoting Laurent Pinchart (2023-06-16 15:09:16)
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Jun 15, 2023 at 11:51:32PM +0100, Kieran Bingham via libcamera-devel wrote:
> > > The CameraManager->get(dev_t) helper was implemented only to support the
> > > V4L2 Adaptation layer, and has been deprecated now that a new camera
> > > property - SystemDevices has been introduced.
> > > 
> > > Rework the implementation of getCameraIndex() to use the SystemDevices
> > > property and remove reliance on the now deprecated call.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >  src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++-----
> > >  1 file changed, 23 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > > index 0f7575c54b22..c49124290b42 100644
> > > --- a/src/v4l2/v4l2_compat_manager.cpp
> > > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > > @@ -24,6 +24,7 @@
> > >  
> > >  #include <libcamera/camera.h>
> > >  #include <libcamera/camera_manager.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "v4l2_camera_file.h"
> > >  
> > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd)
> > >       if (ret < 0)
> > >               return -1;
> > >  
> > > -     std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev);
> > > -     if (!target)
> > > -             return -1;
> > > +     const dev_t devnum = statbuf.st_rdev;
> > 
> >         const int64_t devnum = static_cast<int64_t>(statbuf.st_rdev);
> > 
> > will save you a static_cast<> in the loop below. Up to you.

I'd rather keep it clear that statbuf.st_rdev is a dev_t. There's not a
great deal of difference in a cast here or a cast below otherwise.

> > 
> > >  
> > > +     /*
> > > +      * Iterate each known camera and identify if it reports this nodes
> > > +      * device number in its list of SystemDevices.
> > > +      */
> > >       auto cameras = cm_->cameras();
> > >       for (auto [index, camera] : utils::enumerate(cameras)) {
> > > -             if (camera == target)
> > > -                     return index;
> > > +             const auto devices = camera->properties()
> > > +                                          .get(properties::SystemDevices)
> > > +                                          .value_or(std::vector<int64_t>{});
> > 
> > Can we be explicit ?
> > 
> >                 const std::vector<int64_t> devices = camera->properties()
> >                                         .get(properties::SystemDevices)
> >                                         .value_or(std::vector<int64_t>{});

I'll use the following (Thanks to Barnabás)

diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
index c49124290b42..fa3cb01bb798 100644
--- a/src/v4l2/v4l2_compat_manager.cpp
+++ b/src/v4l2/v4l2_compat_manager.cpp
@@ -122,9 +122,9 @@ int V4L2CompatManager::getCameraIndex(int fd)
 	 */
 	auto cameras = cm_->cameras();
 	for (auto [index, camera] : utils::enumerate(cameras)) {
-		const auto devices = camera->properties()
+		Span<const int64_t> devices = camera->properties()
 					     .get(properties::SystemDevices)
-					     .value_or(std::vector<int64_t>{});
+					     .value_or(Span<int64_t>{});

 		/*
 		 * While there may be multiple Cameras that could reference the
@@ -136,9 +136,10 @@ int V4L2CompatManager::getCameraIndex(int fd)
 		 * hide all devices that may be used by libcamera and expose a
 		 * consistent 1:1 mapping with each Camera instance.
 		 */
-		for (const int64_t dev : devices)
+		for (const int64_t dev : devices) {
 			if (dev == static_cast<int64_t>(devnum))
 				return index;
+		}
 	}

 	return -1;


> > 
> > > +
> > > +             /*
> > > +              * While there may be multiple Cameras that could reference the
> > > +              * same device node, we take a first match as a best effort for
> > > +              * now.
> > > +              *
> > > +              * \todo Consider reworking the V4L2 adaptation layer to stop
> > > +              * trying to map video nodes directly to a camera and instead
> > > +              * hide all devices that may be used by libcamera and expose a
> > > +              * consistent 1:1 mapping with each Camera instance.
> > 
> > I'm not sure what you mean here, could you elaborate ?
> 
> What I mean is I think this could all be improved. In particular to stop
> the issue where if mulitple cameras reference the same video node
> (unicam for instance) they would only ever work for the first one.
> 
> If there were time on this I would investigate making this layer
> provides a better mapping where each camera is /dev/videoN directly.
> 
> 
> 
> > > +              */
> > > +             for (const int64_t dev : devices)
> > > +                     if (dev == static_cast<int64_t>(devnum))
> > > +                             return index;
> > 
> >                 for (const int64_t dev : devices) {
> >                         if (dev == static_cast<int64_t>(devnum))
> >                                 return index;
> >                 }
> > 
> > >       }
> > >  
> > >       return -1;
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart