[libcamera-devel] [PATCH v5 3/5] libcamera: camera: Generate camera name from a CameraSensor
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Aug 3 01:04:17 CEST 2020
Hi Laurent,
Thanks for your feedback.
On 2020-08-02 23:39:04 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Wed, Jul 29, 2020 at 01:50:53PM +0200, Niklas Söderlund wrote:
> > Add a create() that generates a camera name from information in a
> > CameraSensor. The intention is to help pipeline handlers that already
> > uses CameraSensor to not have to worry about how to generate a unique
> > and persistent name.
> >
> > All pipelines that use a CameraSensor (all but UVC) are updated to make
> > use of this new function. All names of cameras created by these updated
> > pipelines are modified.
> >
> > Before this change example of camera names:
> >
> > * OF based systems
> > ov5695 7-0036
> > ov2685 7-003c
> >
> > * ACPI based systems
> > ov13858 8-0010
> > ov5670 10-0036
> >
> > After this change the same cameras are:
> >
> > * OF based systems
> > base/i2c at ff160000/camera at 36
> > base/i2c at ff160000/camera at 3c
> >
> > * ACPI based systems
> > \_SB_.PCI0.I2C2.CAM0
> > \_SB_.PCI0.I2C4.CAM1
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > * Changes since v3
> > - Update commit message.
> > - Do not include property_ids.h.
> > ---
> > include/libcamera/camera.h | 5 +++++
> > src/libcamera/camera.cpp | 18 ++++++++++++++++++
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++-------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > src/libcamera/pipeline/simple/simple.cpp | 2 +-
> > src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--
> > 7 files changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 4d1a4a9f52ec0fac..784510c9a79d44b9 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -19,6 +19,7 @@
> >
> > namespace libcamera {
> >
> > +class CameraSensor;
> > class FrameBuffer;
> > class FrameBufferAllocator;
> > class PipelineHandler;
> > @@ -73,6 +74,10 @@ public:
> > const std::string &name,
> > const std::set<Stream *> &streams);
> >
> > + static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> > + const CameraSensor *sensor,
> > + const std::set<Stream *> &streams);
> > +
> > Camera(const Camera &) = delete;
> > Camera &operator=(const Camera &) = delete;
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 69a1b4428e3f4eca..02e2369ef0089465 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -14,6 +14,7 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/log.h"
> > #include "libcamera/internal/pipeline_handler.h"
> > #include "libcamera/internal/utils.h"
> > @@ -473,6 +474,23 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> > return std::shared_ptr<Camera>(camera, Deleter());
> > }
> >
> > +/**
> > + * \brief Create a camera instance
> > + * \param[in] pipe The pipeline handler responsible for the camera device
> > + * \param[in] sensor The sensor of the camera device
> > + * \param[in] streams Array of streams the camera provides
> > + *
> > + * Create a camera with name generated from \a sensor.
> > + *
> > + * \return A shared pointer to the newly created camera object
> > + */
> > +std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> > + const CameraSensor *sensor,
> > + const std::set<Stream *> &streams)
> > +{
> > + return Camera::create(pipe, sensor->id(), streams);
> > +}
>
> I think I'd prefer passing the id() directly to the existing create()
> function, to have a single API for pipeline handlers. I agree there's
> pros and cons though, so I can be convinced otherwise.
We can always add this as a helper late. I will remove this for v6.
>
> In any case, the name argument to the existing function should be
> renamed to id, and the documentation updated accordingly to explain what
> the id and and how is has to be unique and stable (if we keep a single
> create() implementation, likely with a mention that the id shall be
> retrieved from a CameraSensor for the pipeline handlers that can do so).
Agreed I will do so in v6.
>
> > +
> > /**
> > * \brief Retrieve the name of the camera
> > * \context This function is \threadsafe.
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index eb00eecfd10a89e4..72da6ed62a7f0de5 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -813,18 +813,16 @@ int PipelineHandlerIPU3::registerCameras()
> > &IPU3CameraData::imguOutputBufferReady);
> >
> > /* Create and register the Camera instance. */
> > - std::string cameraName = cio2->sensor()->entity()->name();
> > - std::shared_ptr<Camera> camera = Camera::create(this,
> > - cameraName,
> > - streams);
> > -
> > - registerCamera(std::move(camera), std::move(data));
> > + std::shared_ptr<Camera> camera =
> > + Camera::create(this, cio2->sensor(), streams);
> >
> > LOG(IPU3, Info)
> > << "Registered Camera[" << numCameras << "] \""
> > - << cameraName << "\""
> > + << camera->name() << "\""
> > << " connected to CSI-2 receiver " << id;
> >
> > + registerCamera(std::move(camera), std::move(data));
> > +
> > numCameras++;
> > }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 82a0a4dfd6824fce..a62dd24b1ab76b87 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -972,7 +972,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > streams.insert(&data->isp_[Isp::Stats]);
> >
> > /* Create and register the camera. */
> > - std::shared_ptr<Camera> camera = Camera::create(this, data->sensor_->model(), streams);
> > + std::shared_ptr<Camera> camera =
> > + Camera::create(this, data->sensor_, streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > return true;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 52a0d862417cc4ec..663e45b109aae9eb 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -971,7 +971,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >
> > std::set<Stream *> streams{ &data->stream_ };
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, sensor->name(), streams);
> > + Camera::create(this, data->sensor_, streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > return 0;
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 0bab5af86f05d63c..1258f81284590060 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -815,7 +815,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > continue;
> >
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, data->sensor_->entity()->name(),
> > + Camera::create(this, data->sensor_.get(),
> > data->streams());
> > registerCamera(std::move(camera), std::move(data));
> > }
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 4f461b928514022d..e1fc087f111b0bc4 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -432,9 +432,9 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> > return false;
> >
> > /* Create and register the camera. */
> > - std::string name{ "VIMC " + data->sensor_->model() };
> > std::set<Stream *> streams{ &data->stream_ };
> > - std::shared_ptr<Camera> camera = Camera::create(this, name, streams);
> > + std::shared_ptr<Camera> camera
> > + = Camera::create(this, data->sensor_, streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > return true;
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list