| Message ID | 20200605120306.25529-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thanks for your work. On 2020-06-05 21:03:06 +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, if the pipeline > handler doesn't report a devnum for a camera, then walk the media device > and entity list and tell the camera manager to map every one of these > devnums to the camera. Out of curiosity how will this work if a pipeline registers two cameras from the same media graph? > > 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. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/camera_manager.h | 3 ++- > src/libcamera/camera_manager.cpp | 14 ++++++++------ > src/libcamera/pipeline_handler.cpp | 12 +++++++++++- > 3 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 079f848a..6095b799 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, > + std::vector<dev_t> devnums); > void removeCamera(Camera *camera); > > static const std::string &version() { return version_; } > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index da806fa7..fa0bd6a0 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, > + 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) > + std::vector<dev_t> devnums) > { > MutexLocker locker(mutex_); > > @@ -183,7 +184,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > > cameras_.push_back(std::move(camera)); > > - if (devnum) { > + for (dev_t devnum : devnums) { > unsigned int index = cameras_.size() - 1; > camerasByDevnum_[devnum] = cameras_[index]; > } > @@ -374,7 +375,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 +386,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, > + std::vector<dev_t> devnums) > { > ASSERT(Thread::current() == p_.get()); > > - p_->addCamera(camera, devnum); > + p_->addCamera(camera, devnums); > } > > /** > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 53aeebdc..b3824a5f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, > data->camera_ = camera.get(); > cameraData_[camera.get()] = std::move(data); > cameras_.push_back(camera); > - manager_->addCamera(std::move(camera), devnum); > + > + std::vector<dev_t> devnums; > + if (devnum != 0) > + devnums.push_back(devnum); > + else > + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) > + for (const MediaEntity *entity : media->entities()) > + devnums.push_back(makedev(entity->deviceMajor(), > + entity->deviceMinor())); > + > + manager_->addCamera(std::move(camera), devnums); > } > > /** > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Fri, Jun 05, 2020 at 07:57:29PM +0200, Niklas Söderlund wrote: > Hi Paul, > > Thanks for your work. > > On 2020-06-05 21:03:06 +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, if the pipeline > > handler doesn't report a devnum for a camera, then walk the media device > > and entity list and tell the camera manager to map every one of these > > devnums to the camera. > > Out of curiosity how will this work if a pipeline registers two cameras > from the same media graph? This is something I've thought about recently. Given that those cameras will most likely not be usable at the same time (at least with the pipeline handlers we have now), we could select them with VIDIOC_S_INPUT through a single emulated V4L2 device. That's not a perfect solution, but I think it could be a good first step. We may want, longer term, to support concurrent usage of cameras handled by the same pipeline handler instance, and that will require pipeline handlers to give a camera to devnode mapping. I would however like to avoid doing so explicitly in all pipeline handlers, so I'm considering an optional argument to PipelineHandler::registerCamera() to provide explicit mapping, and otherwise map to all the capture video nodes. Just brainstorming here, so please feel free to propose other options. > > 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. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > include/libcamera/camera_manager.h | 3 ++- > > src/libcamera/camera_manager.cpp | 14 ++++++++------ > > src/libcamera/pipeline_handler.cpp | 12 +++++++++++- > > 3 files changed, 21 insertions(+), 8 deletions(-) > > > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > > index 079f848a..6095b799 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, > > + std::vector<dev_t> devnums); > > void removeCamera(Camera *camera); > > > > static const std::string &version() { return version_; } > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index da806fa7..fa0bd6a0 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, > > + 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) > > + std::vector<dev_t> devnums) > > { > > MutexLocker locker(mutex_); > > > > @@ -183,7 +184,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > > > > cameras_.push_back(std::move(camera)); > > > > - if (devnum) { > > + for (dev_t devnum : devnums) { > > unsigned int index = cameras_.size() - 1; > > camerasByDevnum_[devnum] = cameras_[index]; > > } > > @@ -374,7 +375,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 +386,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, > > + std::vector<dev_t> devnums) > > { > > ASSERT(Thread::current() == p_.get()); > > > > - p_->addCamera(camera, devnum); > > + p_->addCamera(camera, devnums); > > } > > > > /** > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 53aeebdc..b3824a5f 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, > > data->camera_ = camera.get(); > > cameraData_[camera.get()] = std::move(data); > > cameras_.push_back(camera); > > - manager_->addCamera(std::move(camera), devnum); > > + > > + std::vector<dev_t> devnums; > > + if (devnum != 0) > > + devnums.push_back(devnum); > > + else > > + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) > > + for (const MediaEntity *entity : media->entities()) > > + devnums.push_back(makedev(entity->deviceMajor(), > > + entity->deviceMinor())); > > + > > + manager_->addCamera(std::move(camera), devnums); > > } > > > > /** > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thank you for the patch. On Fri, Jun 05, 2020 at 09:03: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, if the pipeline > handler doesn't report a devnum for a camera, then walk the media device > and entity list and tell the camera manager to map every one of these > devnums to the camera. > > 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. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/camera_manager.h | 3 ++- > src/libcamera/camera_manager.cpp | 14 ++++++++------ > src/libcamera/pipeline_handler.cpp | 12 +++++++++++- > 3 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 079f848a..6095b799 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, > + std::vector<dev_t> devnums); Make this a const std::vector<> & to avoid copies. > void removeCamera(Camera *camera); > > static const std::string &version() { return version_; } > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index da806fa7..fa0bd6a0 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, > + std::vector<dev_t> devnums); Same here. > void removeCamera(Camera *camera); > > /* > @@ -168,7 +169,7 @@ void CameraManager::Private::cleanup() > } > > void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > - dev_t devnum) > + std::vector<dev_t> devnums) > { > MutexLocker locker(mutex_); > > @@ -183,7 +184,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > > cameras_.push_back(std::move(camera)); > > - if (devnum) { > + for (dev_t devnum : devnums) { > unsigned int index = cameras_.size() - 1; You can move this outside of the loop. > camerasByDevnum_[devnum] = cameras_[index]; > } > @@ -374,7 +375,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 +386,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, > + std::vector<dev_t> devnums) > { > ASSERT(Thread::current() == p_.get()); > > - p_->addCamera(camera, devnum); > + p_->addCamera(camera, devnums); > } > > /** > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 53aeebdc..b3824a5f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, > data->camera_ = camera.get(); > cameraData_[camera.get()] = std::move(data); > cameras_.push_back(camera); > - manager_->addCamera(std::move(camera), devnum); > + > + std::vector<dev_t> devnums; > + if (devnum != 0) > + devnums.push_back(devnum); Should we allow this, or always use all the capture video nodes ? > + else > + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) > + for (const MediaEntity *entity : media->entities()) > + devnums.push_back(makedev(entity->deviceMajor(), > + entity->deviceMinor())); You need to limit entities to the capture video nodes. You can handle that through a combination of entity flags (to check if it's a video node) and pad flags (to check if it's a capture video node, by looking at the direction of the pad). A short comment to explain what's going on would be useful. > + > + manager_->addCamera(std::move(camera), devnums); > } > > /**
Hi Laurent, On 2020-06-05 21:57:54 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Fri, Jun 05, 2020 at 07:57:29PM +0200, Niklas Söderlund wrote: > > Hi Paul, > > > > Thanks for your work. > > > > On 2020-06-05 21:03:06 +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, if the pipeline > > > handler doesn't report a devnum for a camera, then walk the media device > > > and entity list and tell the camera manager to map every one of these > > > devnums to the camera. > > > > Out of curiosity how will this work if a pipeline registers two cameras > > from the same media graph? > > This is something I've thought about recently. Given that those cameras > will most likely not be usable at the same time (at least with the > pipeline handlers we have now), we could select them with VIDIOC_S_INPUT > through a single emulated V4L2 device. That's not a perfect solution, > but I think it could be a good first step. We may want, longer term, to > support concurrent usage of cameras handled by the same pipeline handler > instance, and that will require pipeline handlers to give a camera to > devnode mapping. I would however like to avoid doing so explicitly in > all pipeline handlers, so I'm considering an optional argument to > PipelineHandler::registerCamera() to provide explicit mapping, and > otherwise map to all the capture video nodes. Just brainstorming here, > so please feel free to propose other options. I'm fine with this approach. Using VIDIOC_S_INPUT sounds like a nice idea going forward as we could then possibly remove the optional devnode mapping argument removing one of the many things a pipeline handler is responsible to get right :-) For this patch with the possible ways we can take it in the future, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > 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. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > include/libcamera/camera_manager.h | 3 ++- > > > src/libcamera/camera_manager.cpp | 14 ++++++++------ > > > src/libcamera/pipeline_handler.cpp | 12 +++++++++++- > > > 3 files changed, 21 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > > > index 079f848a..6095b799 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, > > > + std::vector<dev_t> devnums); > > > void removeCamera(Camera *camera); > > > > > > static const std::string &version() { return version_; } > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > > index da806fa7..fa0bd6a0 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, > > > + 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) > > > + std::vector<dev_t> devnums) > > > { > > > MutexLocker locker(mutex_); > > > > > > @@ -183,7 +184,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, > > > > > > cameras_.push_back(std::move(camera)); > > > > > > - if (devnum) { > > > + for (dev_t devnum : devnums) { > > > unsigned int index = cameras_.size() - 1; > > > camerasByDevnum_[devnum] = cameras_[index]; > > > } > > > @@ -374,7 +375,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 +386,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, > > > + std::vector<dev_t> devnums) > > > { > > > ASSERT(Thread::current() == p_.get()); > > > > > > - p_->addCamera(camera, devnum); > > > + p_->addCamera(camera, devnums); > > > } > > > > > > /** > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 53aeebdc..b3824a5f 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, > > > data->camera_ = camera.get(); > > > cameraData_[camera.get()] = std::move(data); > > > cameras_.push_back(camera); > > > - manager_->addCamera(std::move(camera), devnum); > > > + > > > + std::vector<dev_t> devnums; > > > + if (devnum != 0) > > > + devnums.push_back(devnum); > > > + else > > > + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) > > > + for (const MediaEntity *entity : media->entities()) > > > + devnums.push_back(makedev(entity->deviceMajor(), > > > + entity->deviceMinor())); > > > + > > > + manager_->addCamera(std::move(camera), devnums); > > > } > > > > > > /** > > > -- > > > 2.20.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Thank you for the review. On Fri, Jun 05, 2020 at 10:02:37PM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > <snip> > > /** > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 53aeebdc..b3824a5f 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, > > data->camera_ = camera.get(); > > cameraData_[camera.get()] = std::move(data); > > cameras_.push_back(camera); > > - manager_->addCamera(std::move(camera), devnum); > > + > > + std::vector<dev_t> devnums; > > + if (devnum != 0) > > + devnums.push_back(devnum); > > Should we allow this, or always use all the capture video nodes ? It doesn't seem to me that applications are very likely to use CameraManager::get(devnum) to get cameras, and only those that deal with V4L2 to a certain extent (eg. the V4L2 compatibility layer), will. In that sense, I think that it won't be bad to disallow declaring a devnum and always map the devnums automatically like below. In addition, having all the capture video nodes mapped to a camera would be prevent confusion on the rules surrounding which "video node is mapped to which camera when using the V4L2 compatibility layer". On the other hand, I think it's good to still provide the ability to pipeline handlers to declare their own mapping if they want to. The default (ie. less work) is the auto-mapping anyway. This might cause confusion to users of the V4L2 compatilibity layer, if they expect that all capture nodes can be used when the pipeline handler has disagreed. So it's ease of use for the users of the V4L2 compatiblity layer, or freedom to pipeline handlers. Maybe user-friendliness is more important, and the pipeline handlers don't care to declare devnum mappings. I think I'll go with the former then, until there are objections. > > + else > > + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) > > + for (const MediaEntity *entity : media->entities()) > > + devnums.push_back(makedev(entity->deviceMajor(), > > + entity->deviceMinor())); > > You need to limit entities to the capture video nodes. You can handle > that through a combination of entity flags (to check if it's a video > node) and pad flags (to check if it's a capture video node, by looking > at the direction of the pad). > > A short comment to explain what's going on would be useful. ack > > + > > + manager_->addCamera(std::move(camera), devnums); > > } > > > > /** Thanks, Paul
Hi Paul, On Sat, Jun 06, 2020 at 04:48:34PM +0900, Paul Elder wrote: > Hi Laurent, > > Thank you for the review. > > On Fri, Jun 05, 2020 at 10:02:37PM +0300, Laurent Pinchart wrote: > > Hi Paul, > > > > Thank you for the patch. > > > > <snip> > > > > /** > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 53aeebdc..b3824a5f 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, > > > data->camera_ = camera.get(); > > > cameraData_[camera.get()] = std::move(data); > > > cameras_.push_back(camera); > > > - manager_->addCamera(std::move(camera), devnum); > > > + > > > + std::vector<dev_t> devnums; > > > + if (devnum != 0) > > > + devnums.push_back(devnum); > > > > Should we allow this, or always use all the capture video nodes ? > > It doesn't seem to me that applications are very likely to use > CameraManager::get(devnum) to get cameras, and only those that deal with > V4L2 to a certain extent (eg. the V4L2 compatibility layer), will. In > that sense, I think that it won't be bad to disallow declaring a devnum > and always map the devnums automatically like below. In addition, having > all the capture video nodes mapped to a camera would be prevent > confusion on the rules surrounding which "video node is mapped to which > camera when using the V4L2 compatibility layer". > > On the other hand, I think it's good to still provide the ability to > pipeline handlers to declare their own mapping if they want to. The > default (ie. less work) is the auto-mapping anyway. This might cause > confusion to users of the V4L2 compatilibity layer, if they expect that > all capture nodes can be used when the pipeline handler has disagreed. > > So it's ease of use for the users of the V4L2 compatiblity layer, or > freedom to pipeline handlers. Maybe user-friendliness is more important, > and the pipeline handlers don't care to declare devnum mappings. I think > I'll go with the former then, until there are objections. Sounds good to me. We may reconsider this later when (if) we'll have pipelines supporting multiple cameras concurrently, but for now I think it's totally fine. > > > + else > > > + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) > > > + for (const MediaEntity *entity : media->entities()) > > > + devnums.push_back(makedev(entity->deviceMajor(), > > > + entity->deviceMinor())); > > > > You need to limit entities to the capture video nodes. You can handle > > that through a combination of entity flags (to check if it's a video > > node) and pad flags (to check if it's a capture video node, by looking > > at the direction of the pad). > > > > A short comment to explain what's going on would be useful. > > ack > > > > + > > > + manager_->addCamera(std::move(camera), devnums); > > > } > > > > > > /**
diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 079f848a..6095b799 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, + std::vector<dev_t> devnums); void removeCamera(Camera *camera); static const std::string &version() { return version_; } diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index da806fa7..fa0bd6a0 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, + 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) + std::vector<dev_t> devnums) { MutexLocker locker(mutex_); @@ -183,7 +184,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera, cameras_.push_back(std::move(camera)); - if (devnum) { + for (dev_t devnum : devnums) { unsigned int index = cameras_.size() - 1; camerasByDevnum_[devnum] = cameras_[index]; } @@ -374,7 +375,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 +386,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, + std::vector<dev_t> devnums) { ASSERT(Thread::current() == p_.get()); - p_->addCamera(camera, devnum); + p_->addCamera(camera, devnums); } /** diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 53aeebdc..b3824a5f 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera, data->camera_ = camera.get(); cameraData_[camera.get()] = std::move(data); cameras_.push_back(camera); - manager_->addCamera(std::move(camera), devnum); + + std::vector<dev_t> devnums; + if (devnum != 0) + devnums.push_back(devnum); + else + for (const std::shared_ptr<MediaDevice> media : mediaDevices_) + for (const MediaEntity *entity : media->entities()) + devnums.push_back(makedev(entity->deviceMajor(), + entity->deviceMinor())); + + manager_->addCamera(std::move(camera), devnums); } /**
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, if the pipeline handler doesn't report a devnum for a camera, then walk the media device and entity list and tell the camera manager to map every one of these devnums to the camera. 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. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- include/libcamera/camera_manager.h | 3 ++- src/libcamera/camera_manager.cpp | 14 ++++++++------ src/libcamera/pipeline_handler.cpp | 12 +++++++++++- 3 files changed, 21 insertions(+), 8 deletions(-)