| Message ID | 20230515124550.3601128-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Tested-by: Ashok Sidipotu <ashok.sidipotu@collabora.com> On Monday, May 15, 2023 18:15 IST, Kieran Bingham <kieran.bingham@ideasonboard.com> 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@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" #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>{}); + 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)); } /** -- 2.34.1
Hi Kieran, Thank you for the patch. 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. Now that I read this, I wonder if we should call the property SystemDevices. It seems clearer to me but I'm not sure why :-) > This facilitates camera daemons or other systems to identify which > devices are being managed by libcamera, and can prevent duplication of > camera resources. Is it really duplication of resources, or resource usage conflicts ? > 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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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" > #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>{}); > + > 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)); > } > > /**
Quoting Laurent Pinchart (2023-06-06 17:27:36) > Hi Kieran, > > Thank you for the patch. > > 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. > > Now that I read this, I wonder if we should call the property > SystemDevices. It seems clearer to me but I'm not sure why :-) man dev_t: dev_t Include: <sys/types.h>. Alternatively, <sys/stat.h>. Used for device IDs. According to POSIX, it shall be an integer type. For further details of this type, see makedev(3). Conforming to: POSIX.1-2001 and later. See also: mknod(2), stat(2) or man makedev: NAME makedev, major, minor - manage a device number DESCRIPTION A device ID consists of two parts: a major ID, identifying the class of the device, and a minor ID, identifying a specific instance of a device in that class. A device ID is represented using the type dev_t. Given major and minor device IDs, makedev() combines these to produce a device ID, returned as the function result. This device ID can be given to mknod(2), for example. The major() and minor() functions perform the converse task: given a device ID, they return, respectively, the major and minor components. These macros can be useful to, for example, decompose the device IDs in the structure returned by stat(2). The value we store is only ever referenced as a 'device number'. They help identify device nodes. They are part of the 'system' but then so is everything else. It may help if it clarifies these are 'internal (linux) system devices' but what is a device in a linux system if it's not a system device? IOW : If you want to ask for this to be SystemDevices, then that's fine. - but it doesn't add anything to me.. > > > This facilitates camera daemons or other systems to identify which > > devices are being managed by libcamera, and can prevent duplication of > > camera resources. > > Is it really duplication of resources, or resource usage conflicts ? Well maybe it's both. It's means that resources are duplicated to the user. They don't conflict unless they are both accessed at the same time. > > 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@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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" > > #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>{}); > > + > > 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)); > > } > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Wed, Jun 07, 2023 at 03:23:48PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2023-06-06 17:27:36) > > 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. > > > > Now that I read this, I wonder if we should call the property > > SystemDevices. It seems clearer to me but I'm not sure why :-) > > man dev_t: > > dev_t > Include: <sys/types.h>. Alternatively, <sys/stat.h>. > > Used for device IDs. According to POSIX, it shall be an integer type. > For further details of this type, see makedev(3). > > Conforming to: POSIX.1-2001 and later. > > See also: mknod(2), stat(2) > > > or > > man makedev: > > NAME > makedev, major, minor - manage a device number > DESCRIPTION > > A device ID consists of two parts: a major ID, identifying the > class of the device, and a minor ID, identifying a specific > instance of a device in that class. A device ID is represented > using the type dev_t. > > Given major and minor device IDs, makedev() combines these to > produce a device ID, returned as the function result. This > device ID can be given to mknod(2), for example. > > The major() and minor() functions perform the converse task: > given a device ID, they return, respectively, the major and minor > components. These macros can be useful to, for example, > decompose the device IDs in the structure returned by stat(2). > > The value we store is only ever referenced as a 'device number'. They > help identify device nodes. They are part of the 'system' but then so > is everything else. > > It may help if it clarifies these are 'internal (linux) system devices' > but what is a device in a linux system if it's not a system device? > > IOW : If you want to ask for this to be SystemDevices, then that's fine. > - but it doesn't add anything to me.. "device" is one of those terms that I really dislike when unqualified, as it's very ambiguous. A camera is a device. A chip in the camera hardware pipeline is a device. A DT node is a device, and it results in a device being created in the kernel. This in turn causes yet another type of devices to be exposed to userspace through a device node. Try to forget most of the things you know about the kernel internals, and think about how an application developer who has no idea how the kernel and UAPIs work will guess what the property exposes just based on its name. Then add the property description. It's totally understandable to someone who already knows what this is about, but less so for other readers. It's not our job to turn the libcamera documentation into a Linux 101, but I think the current property name and short documentation could be improved. > > > This facilitates camera daemons or other systems to identify which > > > devices are being managed by libcamera, and can prevent duplication of > > > camera resources. > > > > Is it really duplication of resources, or resource usage conflicts ? > > Well maybe it's both. It's means that resources are duplicated to the > user. They don't conflict unless they are both accessed at the same > time. This is "just" the commit message so I won't bikeshed too much at this time of the night :-) > > > 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@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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" > > > #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>{}); > > > + > > > 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)); > > > } > > > > > > /**
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@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 ? > 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@ideasonboard.com> Thanks j > } > > /** > -- > 2.34.1 >
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@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@ideasonboard.com> > > Thanks > j > > > } > > > > /** > > -- > > 2.34.1 > >
Quoting Kieran Bingham (2023-06-15 13:41:16) > 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@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 This gets through checkstyle, while > > > > auto devnums = camera->properties().get(properties::Devices) > > .value_or(Span<int64_t>{}); If I apply the above it causes: --- src/libcamera/camera_manager.cpp +++ src/libcamera/camera_manager.cpp @@ -178,8 +178,7 @@ } } - auto devnums = camera->properties().get(properties::SystemDevices) - .value_or(Span<int64_t>{}); + auto devnums = camera->properties().get(properties::SystemDevices).value_or(Span<int64_t>{}); cameras_.push_back(std::move(camera)); To fit all on one line which is longer than 80 chars. I prefer the hanging indent for the chained calls, so I'll leave it as it is in this patch. > > > > ? > > > > 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@ideasonboard.com> > > > > Thanks > > j > > > > > } > > > > > > /** > > > -- > > > 2.34.1 > > >
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" #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>{}); + 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)); } /**
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@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(-)