[libcamera-devel] [PATCH v3 5/5] libcamera: pipeline: Register device numbers with camera

[Kieran Bingham]

Quoting Jacopo Mondi (2023-06-15 12:40:36)
> Hi Kieran
> 
> On Mon, May 15, 2023 at 01:45:50PM +0100, Kieran Bingham via libcamera-devel wrote:
> > Register the identified device numbers with each camera as the Devices
> > property.
> >
> > This facilitates camera daemons or other systems to identify which
> > devices are being managed by libcamera, and can prevent duplication of
> > camera resources.
> >
> > As the Devices property now provides this list of devices, use it
> > directly from within the CameraManager when adding a Camera rather than
> > passing it through the internal API.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_manager.h |  3 +--
> >  src/libcamera/camera_manager.cpp            | 14 +++++++++-----
> >  src/libcamera/pipeline_handler.cpp          | 12 ++++++++++--
> >  3 files changed, 20 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> > index 885bb2825687..680e1f58cb02 100644
> > --- a/include/libcamera/internal/camera_manager.h
> > +++ b/include/libcamera/internal/camera_manager.h
> > @@ -29,8 +29,7 @@ public:
> >       Private();
> >
> >       int start();
> > -     void addCamera(std::shared_ptr<Camera> camera,
> > -                    const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +     void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >       void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >
> >       /*
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 70eb4e455e54..7e499def9ddc 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -15,7 +15,9 @@
> >  #include <libcamera/base/utils.h>
> >
> >  #include <libcamera/camera.h>
> > +#include <libcamera/property_ids.h>
> >
> > +#include "libcamera/internal/camera.h"
> 
> This already includes <libcamera/camera.h>
> 
> doesn't hurt though
> 
> >  #include "libcamera/internal/device_enumerator.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> >
> > @@ -150,19 +152,17 @@ void CameraManager::Private::cleanup()
> >  /**
> >   * \brief Add a camera to the camera manager
> >   * \param[in] camera The camera to be added
> > - * \param[in] devnums The device numbers to associate with \a camera
> >   *
> >   * This function is called by pipeline handlers to register the cameras they
> >   * handle with the camera manager. Registered cameras are immediately made
> >   * available to the system.
> >   *
> > - * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> > - * to Camera instances.
> > + * Device numbers from the Devices property are used by the V4L2 compatibility
> > + * layer to map V4L2 device nodes to Camera instances.
> >   *
> >   * \context This function shall be called from the CameraManager thread.
> >   */
> > -void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> > -                                    const std::vector<dev_t> &devnums)
> > +void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera)
> >  {
> >       ASSERT(Thread::current() == this);
> >
> > @@ -177,6 +177,10 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> >               }
> >       }
> >
> > +     auto devnums = camera->properties()
> > +                            .get(properties::Devices)
> > +                            .value_or(Span<int64_t>{});
> > +
> 
> Weird indent
> 
>         auto devnums = camera->properties().get(properties::Devices)
>                                            .value_or(Span<int64_t>{});
> 
> ?
> 
> I guess we don't need to make sure devnums is populated, right ?

No - if we end up with Virtual pipeline handlers there wouldn't be any
devnums anyway.

I'd expect we can later update the V4L2 Adaptation layer to use the
Property directly and clean up this 'extra registration' too.

I wanted to get the core implementation handled first though.

--
Kieran


> 
> >       cameras_.push_back(std::move(camera));
> >
> >       unsigned int index = cameras_.size() - 1;
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 49092ea88a58..0b5fac7ad113 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -17,6 +17,7 @@
> >
> >  #include <libcamera/camera.h>
> >  #include <libcamera/framebuffer.h>
> > +#include <libcamera/property_ids.h>
> >
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_manager.h"
> > @@ -612,7 +613,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >        * Walk the entity list and map the devnums of all capture video nodes
> >        * to the camera.
> >        */
> > -     std::vector<dev_t> devnums;
> > +     std::vector<int64_t> devnums;
> >       for (const std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> >               for (const MediaEntity *entity : media->entities()) {
> >                       if (entity->pads().size() == 1 &&
> > @@ -624,7 +625,14 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >               }
> >       }
> >
> > -     manager_->_d()->addCamera(std::move(camera), devnums);
> > +     /*
> > +      * Store the associated devices as a property of the camera to allow
> > +      * systems to identify which devices are managed by libcamera.
> > +      */
> > +     Camera::Private *data = camera->_d();
> > +     data->properties_.set(properties::Devices, devnums);
> > +
> > +     manager_->_d()->addCamera(std::move(camera));
> 
> The rest looks good
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> Thanks
>   j
> 
> >  }
> >
> >  /**
> > --
> > 2.34.1
> >