Message ID | 20200606080706.14290-1-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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); > } > > /**
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()); - /* 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()) + if (entity->pads().size() == 1 && + (entity->pads()[0]->flags() & MEDIA_PAD_FL_SINK)) + devnums.push_back(makedev(entity->deviceMajor(), + entity->deviceMinor())); + + manager_->addCamera(std::move(camera), devnums); } /**