[libcamera-devel] [PATCH v3 2/5] libcamera: camera_sensor: Generate a sensor ID

[Jacopo Mondi]

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 ?

> +	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 ?

> +		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 ?

> +
> +	/*
> +	 * 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.

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
  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