[libcamera-devel,v3,2/6] libcamera: pipeline_handler: Add map from devnum to cameras

2445 diff mbox series

The V4L2 compatibility layer will need a way to map device numbers to
libcamera Cameras. As a first step, we add a map from devnum to Cameras
to PipelineHandler, as well as an overloaded registerCamera() method to
add to this map.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v3
---
 src/libcamera/include/pipeline_handler.h |  6 +++++
 src/libcamera/pipeline_handler.cpp       | 30 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Hi Paul,

On Mon, Dec 23, 2019 at 01:26:16AM -0600, Paul Elder wrote:
> The V4L2 compatibility layer will need a way to map device numbers to
> libcamera Cameras. As a first step, we add a map from devnum to Cameras
> to PipelineHandler, as well as an overloaded registerCamera() method to
> add to this map.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> New in v3
> ---
>  src/libcamera/include/pipeline_handler.h |  6 +++++
>  src/libcamera/pipeline_handler.cpp       | 30 ++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index f3622631..563de72a 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -12,6 +12,7 @@
>  #include <memory>
>  #include <set>
>  #include <string>
> +#include <sys/sysmacros.h>
>  #include <vector>
>
>  #include <ipa/ipa_interface.h>
> @@ -84,9 +85,13 @@ public:
>
>  	const char *name() const { return name_; }
>
> +	const std::map<dev_t, std::weak_ptr<Camera>> &camerasByDevnum() const { return camerasByDevnum_; }
> +
>  protected:
>  	void registerCamera(std::shared_ptr<Camera> camera,
>  			    std::unique_ptr<CameraData> data);
> +	void registerCamera(std::shared_ptr<Camera> camera,
> +			    std::unique_ptr<CameraData> data, dev_t devnum);

Just wondering if it would not be better to have a single
implementation with devnum = 0 (it can't be 0, right ? )

>  	void hotplugMediaDevice(MediaDevice *media);
>
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> @@ -102,6 +107,7 @@ private:
>  	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
>  	std::vector<std::weak_ptr<Camera>> cameras_;
>  	std::map<const Camera *, std::unique_ptr<CameraData>> cameraData_;
> +	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
>
>  	const char *name_;
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5badf31c..90211f12 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -7,6 +7,8 @@
>
>  #include "pipeline_handler.h"
>
> +#include <sys/sysmacros.h>
> +
>  #include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
> @@ -453,6 +455,28 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
>  	manager_->addCamera(std::move(camera));
>  }
>
> +/**
> + * \brief Register a camera to the camera manager and pipeline handler

Nit:
If you keep it separate, I would add "Register a camera associated
with \a devnum to... "

> + * \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
> + *
> + * 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.
> + */
> +void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> +				     std::unique_ptr<CameraData> data,
> +				     dev_t devnum)
> +{
> +	registerCamera(camera, std::move(data));
> +	camerasByDevnum_[devnum] = camera;
> +}
> +
>  /**
>   * \brief Enable hotplug handling for a media device
>   * \param[in] media The media device
> @@ -538,6 +562,12 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)
>   * \return The pipeline handler name
>   */
>
> +/**
> + * \fn PipelineHandler::camerasByDevnum()
> + * \brief Retrieve the map of devnums to cameras
> + * \return The map of devnums to cameras

s/devnums/device numbers/ ?

Nit aparts:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> + */
> +
>  /**
>   * \class PipelineHandlerFactory
>   * \brief Registration of PipelineHandler classes and creation of instances
> --
> 2.23.0
>
> _______________________________________________
> 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, Dec 27, 2019 at 01:37:45PM +0100, Jacopo Mondi wrote:
> On Mon, Dec 23, 2019 at 01:26:16AM -0600, Paul Elder wrote:
> > The V4L2 compatibility layer will need a way to map device numbers to
> > libcamera Cameras. As a first step, we add a map from devnum to Cameras

It's no big deal in the commit message, but we don't pluralize class
names ("Cameras") in the documentation as Doxygen would then not
recognize them. This should then be "cameras" or "Camera instances" if
you want to be consistent here.

> > to PipelineHandler, as well as an overloaded registerCamera() method to
> > add to this map.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > New in v3
> > ---
> >  src/libcamera/include/pipeline_handler.h |  6 +++++
> >  src/libcamera/pipeline_handler.cpp       | 30 ++++++++++++++++++++++++
> >  2 files changed, 36 insertions(+)
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index f3622631..563de72a 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -12,6 +12,7 @@
> >  #include <memory>
> >  #include <set>
> >  #include <string>
> > +#include <sys/sysmacros.h>
> >  #include <vector>
> >
> >  #include <ipa/ipa_interface.h>
> > @@ -84,9 +85,13 @@ public:
> >
> >  	const char *name() const { return name_; }
> >
> > +	const std::map<dev_t, std::weak_ptr<Camera>> &camerasByDevnum() const { return camerasByDevnum_; }
> > +
> >  protected:
> >  	void registerCamera(std::shared_ptr<Camera> camera,
> >  			    std::unique_ptr<CameraData> data);
> > +	void registerCamera(std::shared_ptr<Camera> camera,
> > +			    std::unique_ptr<CameraData> data, dev_t devnum);
> 
> Just wondering if it would not be better to have a single
> implementation with devnum = 0 (it can't be 0, right ? )

Correct, 0 is documented as "reserved as null device number". I think a
single implementation is indeed better.

> >  	void hotplugMediaDevice(MediaDevice *media);
> >
> >  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> > @@ -102,6 +107,7 @@ private:
> >  	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> >  	std::vector<std::weak_ptr<Camera>> cameras_;
> >  	std::map<const Camera *, std::unique_ptr<CameraData>> cameraData_;
> > +	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;

Could we do away with storage of the map in the PipelineHandler, as we
have one in the CameraManager ? I think you can just pass the devnum to
the CameraManager::addCamera() method in
PipelineHandler::registerCamera().

> >
> >  	const char *name_;
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 5badf31c..90211f12 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -7,6 +7,8 @@
> >
> >  #include "pipeline_handler.h"
> >
> > +#include <sys/sysmacros.h>
> > +
> >  #include <libcamera/buffer.h>
> >  #include <libcamera/camera.h>
> >  #include <libcamera/camera_manager.h>
> > @@ -453,6 +455,28 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> >  	manager_->addCamera(std::move(camera));
> >  }
> >
> > +/**
> > + * \brief Register a camera to the camera manager and pipeline handler
> 
> Nit:
> If you keep it separate, I would add "Register a camera associated
> with \a devnum to... "
> 
> > + * \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
> > + *
> > + * 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.

Let's expand this slightly, to explain the purpose of the association,
otherwise it's difficult for pipeline handler authors to figure out what
devnum to pass. 

> > + */
> > +void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> > +				     std::unique_ptr<CameraData> data,
> > +				     dev_t devnum)
> > +{
> > +	registerCamera(camera, std::move(data));
> > +	camerasByDevnum_[devnum] = camera;
> > +}
> > +
> >  /**
> >   * \brief Enable hotplug handling for a media device
> >   * \param[in] media The media device
> > @@ -538,6 +562,12 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)
> >   * \return The pipeline handler name
> >   */
> >
> > +/**
> > + * \fn PipelineHandler::camerasByDevnum()
> > + * \brief Retrieve the map of devnums to cameras
> > + * \return The map of devnums to cameras
> 
> s/devnums/device numbers/ ?
> 
> Nit aparts:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > + */
> > +
> >  /**
> >   * \class PipelineHandlerFactory
> >   * \brief Registration of PipelineHandler classes and creation of instances