[libcamera-devel] [PATCH v3 2/5] libcamera: camera_sensor: Generate a sensor ID
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jul 29 10:49:13 CEST 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-07-29 09:56:44 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Wed, Jul 29, 2020 at 01:42:22AM +0200, Niklas Söderlund wrote:
> > Generate a constant and unique string ID for the sensor. The ID is
> > generated from firmware information fetched from the hardware that backs
> > the sensor. The ID is unique and persistent across reboots of the
>
> nit: I would drop "fetched from the hardwaware ..." and replace it
> with "from the firwmare descritpion of the camera image sensor".
>
> > system.
> >
> > For OF based systems the ID is the full path of the sensors in the
> > device tree description. For ACPI bases systems the ID is the ACPI
>
> s/bases/based/
>
> > firmware nodes path. Both ID sources are guaranteed to be unique and
> > persistent as long as the firmware of the system is not changed.
> >
> > A special case is needed to deal with the VIMC pipeline that implements
> > a virtual pipeline that is not backed by any hardware device and is
> > therefore not described in the device firmware.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/internal/camera_sensor.h | 4 +
> > src/libcamera/camera_sensor.cpp | 85 ++++++++++++++++++++++
> > 2 files changed, 89 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 06c8292ca30129de..e0d2d9f63b47c2fe 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -47,6 +47,7 @@ public:
> > int init();
> >
> > const std::string &model() const { return model_; }
> > + const std::string &id() const { return id_; }
> > const MediaEntity *entity() const { return entity_; }
> > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > const std::vector<Size> &sizes() const { return sizes_; }
> > @@ -67,11 +68,14 @@ protected:
> > std::string logPrefix() const override;
> >
> > private:
> > + int generateID();
> > +
> > const MediaEntity *entity_;
> > std::unique_ptr<V4L2Subdevice> subdev_;
> > unsigned int pad_;
> >
> > std::string model_;
> > + std::string id_;
> >
> > V4L2Subdevice::Formats formats_;
> > Size resolution_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 350f49accad99c7b..a4339759bc847ff0 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -9,10 +9,12 @@
> >
> > #include <algorithm>
> > #include <float.h>
> > +#include <fstream>
> > #include <iomanip>
> > #include <limits.h>
> > #include <math.h>
> > #include <regex>
> > +#include <sys/stat.h>
> >
> > #include <libcamera/property_ids.h>
> >
> > @@ -204,6 +206,11 @@ int CameraSensor::init()
> > if (ret < 0)
> > return ret;
> >
> > + /* Generate a unique ID for the sensor. */
> > + ret = generateID();
> > + if (ret)
> > + return ret;
> > +
> > /* Retrieve and store the camera sensor properties. */
> > const ControlInfoMap &controls = subdev_->controls();
> > int32_t propertyValue;
> > @@ -283,6 +290,17 @@ int CameraSensor::init()
> > * \return The sensor model name
> > */
> >
> > +/**
> > + * \fn CameraSensor::id()
> > + * \brief Retrieve the sensor ID
> > + *
> > + * The sensor ID is a free-formed string that uniquely identifies the sensor in
> > + * the system. The ID is persisted between different instances of libcamera and
>
> s/persisted/persistent/
>
> > + * between resets of the system.
>
> You know, I would copy part of the commit message here that tells how
> a sensor ID is composed.
>
> > + *
> > + * \return The sensor ID
> > + */
> > +
> > /**
> > * \fn CameraSensor::entity()
> > * \brief Retrieve the sensor media entity
> > @@ -541,4 +559,71 @@ std::string CameraSensor::logPrefix() const
> > return "'" + entity_->name() + "'";
> > }
> >
> > +int CameraSensor::generateID()
> > +{
> > + std::string path, devPath = subdev_->devicePath();
>
> nit: can devPath be a const reference ?
No, devicePath() returns a value and not a reference.
>
> > + struct stat statbuf;
> > +
> > + /* Try to generate ID from OF device tree path. */
> > + path = devPath + "/of_node";
> > + if (stat(path.c_str(), &statbuf) == 0) {
> > + char ofPath[PATH_MAX];
> > +
> > + if (!realpath(path.c_str(), ofPath)) {
> > + LOG(CameraSensor, Error) << "Failed to read sensor OF based ID";
> > + return -EINVAL;
> > + }
> > +
> > + id_ = ofPath;
> > + const std::string dropStr = "/sys/firmware/devicetree/";
>
> Have you considered dropping '/base' too ?
I thought about it but decided again it. If we ever get proper runtime
DT overlay support which could used to add cameras at runtime we need to
keep the /base prefix to distinguish it as far as I understand it.
>
> > + if (id_.find(dropStr) == 0)
> > + id_.erase(0, dropStr.length());
> > +
> > + return 0;
> > + }
> > +
> > + /* Try to generate ID from ACPI path. */
> > + path = devPath + "/firmware_node/path";
> > + if (stat(path.c_str(), &statbuf) == 0) {
> > + std::ifstream file(path.c_str());
> > +
> > + if (!file.is_open()) {
> > + LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> > + return -EINVAL;
> > + }
> > +
> > + std::getline(file, id_);
> > + file.close();
> > +
> > + return 0;
> > + }
>
> I'm now worried about file permission. Opening a file in RO mode would
> help avoiding permission issues ?
std::ifstream is RO :-)
>
> > +
> > + /*
> > + * VIMC is a virtual video pipeline not backed hardware and have no OF
> > + * or ACPI firmware nodes. Handle this pipeline as a special case and
> > + * generate IDs based on the sensor model.
> > + */
> > + path = devPath + "/modalias";
> > + if (stat(path.c_str(), &statbuf) == 0) {
> > + std::ifstream file(path.c_str());
> > +
> > + if (!file.is_open()) {
> > + LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> > + return -EINVAL;
> > + }
> > +
> > + std::string modalias;
> > + std::getline(file, modalias);
> > + file.close();
> > +
> > + if (modalias == "platform:vimc") {
> > + id_ = "VIMC " + model();
> > + return 0;
> > + }
> > + }
> > +
> > + LOG(CameraSensor, Error) << "Unknown sensor device type, can not generate ID";
>
> I would however set id_ to a default. Maybe with an increasing counter
> appended.
I think that is a bad idea, here we error out with -EINVAL and as this
is a private function the only caller is CameraSensor::init() which will
propagate the error.
I don't think its a good idea to hide and try to recover if we encounter
a new device type, what would that even be? Maybe we should even make
the error fatal as it really should not happen.
>
> This looks very nice to me and a really good way to address this
> thorny issue
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks!
>
> Thanks
> j
>
> > + return -EINVAL;
> > +}
> > +
> > } /* namespace libcamera */
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list