[libcamera-devel] [PATCH v2] libcamera: CameraManager, PipelineHandler: Automatically map devnums to Camera
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 12:54:10 CEST 2020
Hi Paul,
Thank you for the patch.
On Sat, Jun 06, 2020 at 05:07:06PM +0900, Paul Elder wrote:
> The V4L2 compatibility layer uses devnum to match video device nodes to
> libcamera Cameras. Some pipeline handlers don't report a devnum for
> their camera, which prevents the V4L2 compatibility layer from matching
> video device nodes to these cameras. To fix this, we first allow the
> camera manager to map multiple devnums to a camera. Next, we walk the
> media device and entity list and tell the camera manager to map every
> one of these devnums that is a video capture node to the camera.
>
> Since we decided that all video capture nodes that belong to a camera
> can be opened via the V4L2 compatibility layer to map to that camera, it
> would cause confusion for users if some pipeline handlers decided that
> only specific device nodes would map to the camera. To prevent this
> confusion, remove the ability for pipeline handlers to declare their own
> devnum-to-camera mapping. The only pipeline handler that declares the
> devnum mapping is the UVC pipeline handler, so remove the devnum there.
>
> We considered walking the media entity list and taking the devnum from
> just the one with the default flag set, but we found that some drivers
> (eg. vimc) don't set this flag for any entity. Instead, we take all the
> video capture nodes (entities with the sink pad flag set).
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> ---
> Changes in v2:
> - pipeline handlers can no longer declare their own devnum
> mapping.
> - remove the uvc pipeline handler devnum mapping
> - cosmetic changes
> ---
> include/libcamera/camera_manager.h | 3 ++-
> include/libcamera/internal/pipeline_handler.h | 2 +-
> src/libcamera/camera_manager.cpp | 17 ++++++-------
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +---
> src/libcamera/pipeline_handler.cpp | 24 ++++++++++++-------
> 5 files changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..95dc6360 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -34,7 +34,8 @@ public:
> std::shared_ptr<Camera> get(const std::string &name);
> std::shared_ptr<Camera> get(dev_t devnum);
>
> - void addCamera(std::shared_ptr<Camera> camera, dev_t devnum);
> + void addCamera(std::shared_ptr<Camera> camera,
> + const std::vector<dev_t> &devnums);
> void removeCamera(Camera *camera);
>
> static const std::string &version() { return version_; }
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 56968d14..22e629a8 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -91,7 +91,7 @@ public:
>
> protected:
> void registerCamera(std::shared_ptr<Camera> camera,
> - std::unique_ptr<CameraData> data, dev_t devnum = 0);
> + std::unique_ptr<CameraData> data);
> void hotplugMediaDevice(MediaDevice *media);
>
> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index da806fa7..3d055f78 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -36,7 +36,8 @@ public:
> Private(CameraManager *cm);
>
> int start();
> - void addCamera(std::shared_ptr<Camera> &camera, dev_t devnum);
> + void addCamera(std::shared_ptr<Camera> &camera,
> + const std::vector<dev_t> &devnums);
> void removeCamera(Camera *camera);
>
> /*
> @@ -168,7 +169,7 @@ void CameraManager::Private::cleanup()
> }
>
> void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> - dev_t devnum)
> + const std::vector<dev_t> &devnums)
> {
> MutexLocker locker(mutex_);
>
> @@ -183,10 +184,9 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
>
> cameras_.push_back(std::move(camera));
>
> - if (devnum) {
> - unsigned int index = cameras_.size() - 1;
> + unsigned int index = cameras_.size() - 1;
> + for (dev_t devnum : devnums)
> camerasByDevnum_[devnum] = cameras_[index];
> - }
> }
>
> void CameraManager::Private::removeCamera(Camera *camera)
> @@ -374,7 +374,7 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> /**
> * \brief Add a camera to the camera manager
> * \param[in] camera The camera to be added
> - * \param[in] devnum The device number to associate with \a camera
> + * \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
> @@ -385,11 +385,12 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> *
> * \context This function shall be called from the CameraManager thread.
> */
> -void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> +void CameraManager::addCamera(std::shared_ptr<Camera> camera,
> + const std::vector<dev_t> &devnums)
> {
> ASSERT(Thread::current() == p_.get());
>
> - p_->addCamera(camera, devnum);
> + p_->addCamera(camera, devnums);
> }
>
> /**
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index a0749094..396bbe9b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -396,12 +396,10 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> if (data->init(*entity))
> return false;
>
> - dev_t devnum = makedev((*entity)->deviceMajor(), (*entity)->deviceMinor());
> -
I think you can then drop inclusion of sys/sysmacros.h.
> /* Create and register the camera. */
> std::set<Stream *> streams{ &data->stream_ };
> std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> - registerCamera(std::move(camera), std::move(data), devnum);
> + registerCamera(std::move(camera), std::move(data));
>
> /* Enable hot-unplug notifications. */
> hotplugMediaDevice(media);
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 53aeebdc..125e1779 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -481,28 +481,34 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request)
> * \brief Register a camera to the camera manager and pipeline handler
> * \param[in] camera The camera to be added
> * \param[in] data Pipeline-specific data for the camera
> - * \param[in] devnum Device number of the camera (optional)
> *
> * This method is called by pipeline handlers to register the cameras they
> * handle with the camera manager. It associates the pipeline-specific \a data
> * with the camera, for later retrieval with cameraData(). Ownership of \a data
> * is transferred to the PipelineHandler.
> *
> - * \a devnum is the device number (as returned by makedev) that the \a camera
> - * is to be associated with. This is for the V4L2 compatibility layer to map
> - * device nodes to Camera instances based on the device number
> - * registered by this method in \a devnum.
> - *
> * \context This function shall be called from the CameraManager thread.
> */
> void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> - std::unique_ptr<CameraData> data,
> - dev_t devnum)
> + std::unique_ptr<CameraData> data)
> {
> data->camera_ = camera.get();
> cameraData_[camera.get()] = std::move(data);
> cameras_.push_back(camera);
> - manager_->addCamera(std::move(camera), devnum);
> +
> + /*
> + * Walk the entity list and map the devnums of all capture video nodes
> + * to the camera.
> + */
> + std::vector<dev_t> devnums;
> + for (const std::shared_ptr<MediaDevice> media : mediaDevices_)
> + for (const MediaEntity *entity : media->entities())
Even if not technically required, we tend to use curly braces around
statements that have substatements. That would apply to the two for
loops here.
> + if (entity->pads().size() == 1 &&
> + (entity->pads()[0]->flags() & MEDIA_PAD_FL_SINK))
> + devnums.push_back(makedev(entity->deviceMajor(),
> + entity->deviceMinor()));
Checking the number of pads isn't enough, there could be subdevs with a
single pads. You need to check that entity->function() ==
MEDIA_ENT_F_IO_V4L. I would keep the pads size check though, even if
likely redundant, to avoid a crash in case a video node would have no
pads.
> +
> + manager_->addCamera(std::move(camera), devnums);
> }
>
> /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list